Skip to content
This repository was archived by the owner on Aug 17, 2022. It is now read-only.

Chooser scenario #103

Merged
merged 10 commits into from
Apr 17, 2020
Merged

Chooser scenario #103

merged 10 commits into from
Apr 17, 2020

Conversation

fgmccabe
Copy link
Contributor

This is an initial draft.
There are some variations on instructions and some instruction renaming.

Copy link
Member

@binji binji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

local.get $left
invoke-func $stralloc-x
invoke-func $shared-x ;; make it a shared ptr
let (local $lpo owned<i32>) (result string)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: current function-references proposal requires this the other way around:

let (result string) (local $lpo owned<i32>)

TBH, I think it's more clear the way you have it, but I thought I'd mention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it reads more naturally to put results second, making let more like a function signature where params have been replaced by locals. Maybe we should change the function-references proposal...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but there's a subtlety to let that I had missed. let introduces a new block, so it has a blocktype, which means w/ the multi-value proposal it's possible to have params and locals:

f32.const 1
i32.const 2
let (param i32) (result i64) (local f32)
  ;; i32:2 is still on top of the stack
  ;; f32:1 was consumed from the stack, and is now local 0
  ...
end

I'm not sure there's value in having let params, but it does feel a bit odd to have the behavior differ significantly from the way other blocks work. It also means that a let block doesn't work at all like the locals at the beginning of a function (which means function inlining doesn't work naturally either).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that too. May need to revisit.

Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally right. I made a few small requested tweaks.

Although I think it's valuable to show the final fused state, I do sortof question the value of showing all the intermediate states. What would be more valuable and general, I think, is to write a separate doc describing semi-formally the fusion algorithm, and then this doc could say "applying the [fusion] algorithm produces this final fused form", and then maybe have a bullet list that points out specific items of interest, like how the own expressions have been moved, etc.

local.get $left
invoke-func $stralloc-x
invoke-func $shared-x ;; make it a shared ptr
let (local $lpo owned<i32>) (result string)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it reads more naturally to put results second, making let more like a function signature where params have been replaced by locals. Maybe we should change the function-references proposal...

@lukewagner
Copy link
Member

Also: great job! This was a lot of work.

Copy link

@ajklein ajklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good to me. I'll say I had to read it twice before I got it, there's a lot going on here.

The thing that definitely needs addressing (beyond typos) is the seemingly unrelated changes.

@@ -1,292 +0,0 @@
# Interface instructions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like something weird happened here; unrelated files getting deleted as part of this work. Maybe this branch isn't up-to-date with master?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is because these files were accidentally included once. I deleted them in a subsequent commit but github fu is hard to figure out

local.get $lsize
memory.copy "mem-i" "mem-x" ;; copy string across
local.get $ptr
i32.const 1 ;; initial reference count of 1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove this reference count above, remove it here too (and below).

Copy link

@ajklein ajklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm; you might also want to remove the .gitignore change from this patch (it looks unrelated)

@fgmccabe fgmccabe merged commit 29bb677 into master Apr 17, 2020
@lukewagner lukewagner deleted the chooser branch September 21, 2021 19:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants