Conversation
Read label names, and use the names when building the IR. Blocks and loops can hold names directly. `if`, `try` and `try_table` cannot, so for them the name is only a hint: we use it when a branch to the scope means we have to create a wrapper block anyhow, and otherwise drop it, so that reading these names never changes the shape of the IR.
cb552ac to
7ddcda1
Compare
Write the label subsection (id 3) next to the function and local names when debug info is enabled. Only explicit names are written: the ones that came from the text format, from the name section, or through the C API, and not the ones we generate ourselves. Functions therefore gain an `explicitLabelNames` set, filled by IRBuilder when a label comes from outside rather than from makeFresh(). Keeping this next to `localNames` and `debugLocations` rather than on the expressions themselves is how the rest of our debug info is stored, and it has two advantages: ExpressionAnalyzer never sees it, so two otherwise-identical blocks cannot stop comparing equal because of debug info; and since label names are unique within a function, it keeps identifying the right labels as optimizations replace the expressions that carry them.
7ddcda1 to
01f2737
Compare
| void writeExpression(Expression* curr); | ||
| void writeFunctions(); | ||
| void noteLabelNames(Function* func, | ||
| std::vector<std::pair<Index, Name>>& labelNames); |
There was a problem hiding this comment.
This is a helper, not one of the core write* methods, so perhaps let's move it to a less prominent place? (maybe around line 1585, the end of the class) Also, it could use a comment as to what it does and what the parameters mean.
| ;; CHECK-NEXT: (block $label | ||
| ;; CHECK-NEXT: (try $try1 | ||
| ;; CHECK-NEXT: (block $try1 | ||
| ;; CHECK-NEXT: (try |
There was a problem hiding this comment.
Why are there changes in this file? I don't seem to see it writing a binary, so the names section changes should not apply..?
There was a problem hiding this comment.
I have made some changes in the IRBuilder to better place labels. For try, we have the choice of putting the label on the wrapper block (as is done for if and try_table) or on the try itself. When there are only branches targeting it (no delegate or rethrow), I think it makes more sense to put the label on the wrapper block: that's why we now have (block $try1), and (br_if $try1) instead of (br_if $label), in this file. Since the IRBuilder is shared between the binary and the text parser, this impacts both.
I can remove this change from this PR and always put the label on the try if you prefer.
There was a problem hiding this comment.
I see, thanks. Please move from it from this PR, then, to keep things simple, and that other PR nice and focused. This is already a large (but useful!) change.
There was a problem hiding this comment.
I have removed the change from the PR.
| // end up needing a label anyhow. | ||
| auto name = getNextLabelName(); | ||
| auto result = builder.makeIf(Name(), getBlockType()); | ||
| builder.setScopeNameHint(name); |
There was a problem hiding this comment.
makeIf accepts a name, and at a glance, seems to use it as best it can. Why can we not pass in the name here directly, rather than sending Name() and then calling setScopeNameHint? (Should makeIf call that method..?)
There was a problem hiding this comment.
Passing a name to makeIf inserts a wrapper block to hold the label. So, the IR would change depending on whether we read the name section or not.
If we change makeIf to treat the label as a hint, this will impact the text parser: unused labels will get dropped.
There was a problem hiding this comment.
I'd be ok with makeIf treating the passed label as a hint. I don't see why we might want to synthesize a block with an unused label just because the label was present in the input.
There was a problem hiding this comment.
I can do that in a follow-up PR.
| if (!scope.label) { | ||
| scope.label = makeFresh(label); | ||
| } | ||
| scope.labelExplicit = true; |
There was a problem hiding this comment.
Don't we push scopes in various situations? Why is the label here always explicit?
There was a problem hiding this comment.
Indeed! Outlining calls visit functions, which push scopes, based on existing expressions whose labels may be generated internally. I have moved the marking to the make functions which are only called from the parsers.
| (do (nop)) | ||
| (catch_all (nop)) | ||
| ) | ||
| ;; A branch to a try needs a wrapper block, which takes the name. |
There was a problem hiding this comment.
It doesn't look like this is what is in the output? I see
(block $block
(try $branched-try
There was a problem hiding this comment.
(the block didn't "take" the name - unless I don't understand what "take" means here)
There was a problem hiding this comment.
Right. The comment was stale.
| @@ -0,0 +1,292 @@ | |||
| ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. | |||
| ;; Test that we write explicit label names to the name section with -g, and | |||
| ;; that we do not write the names we generate ourselves. | |||
There was a problem hiding this comment.
I am not immediately clear on how to verify this in the test below. Perhaps annotate places with "here a name will be generated ourselves, and it will [..]" (the last part is what confuses me: even if we generate it ourselves, we generated it for a reason, so it will show up in the output? so how can we tell what is in the name section or not, just by reading the reloaded wat..?)
There was a problem hiding this comment.
This file now only checks that the names are preserved.
In test/lit/binary/label-names-generated.wast, one can see that the label $__inlined_func$callee appears in the text output but not after a round trip through the binary.
I'm not sure how to do better, since we don't have any easy way to print the name section.
There was a problem hiding this comment.
How about directly inspecting the name section? We can write a unit test in python that parses the first few bytes of the name section, just enough to see there is only 1 name, and grep finds it in the wasm? (that would prove all other names are not in there)
There was a problem hiding this comment.
in node, .customSection is an easy way to get the section bytes, so that is another option.
There was a problem hiding this comment.
I think leaving comments in this file about the generated names (or combining the test files and/or having a third check prefix showing the directly emitted wat with generated names here) would be better than adding a new python unit test or using node.
| ;; RUN: wasm-dis %t.nodebug.wasm -all -o - | filecheck %s --check-prefix=CHECK-BIN-NODEBUG | ||
|
|
||
| (module | ||
| ;; INLINE: (type $0 (func (param i32) (result i32))) |
There was a problem hiding this comment.
It looks like the INLINE checks might be stale; they don't have a corresponding RUN line.
This is the only section of the Extended Name Section proposal which is currently not handled.