Reconstruct locations of the original source variable #698
Conversation
yurydelendik
force-pushed
the
get-val-labels
branch
from
46770fd to
c2ee106
Compare
yurydelendik
force-pushed
the
get-val-labels
branch
from
251d23a to
ee36aa3
Compare
|
Could you please add cc bjorn3/rustc_codegen_cranelift#166. |
|
yurydelendik
force-pushed
the
get-val-labels
branch
2 times, most recently
from
acc17cf to
346ebe6
Compare
yurydelendik
changed the title
| /// Marked with a label value. | ||
| #[derive(Copy, Clone, PartialEq, Eq, Hash)] | ||
| pub struct ValueLabel(u32); | ||
| entity_impl!(ValueLabel, "val"); |
There was a problem hiding this comment.
Would it work to use Variable instead of defining a new index type? In the code below, it always has the same value as the Variable.
There was a problem hiding this comment.
I was thinking to extend ValueLabel to be used for global or stack operands as well.
| state.push1(builder.use_var(Variable::with_u32(local_index))) | ||
| state.push1(builder.use_var(Variable::with_u32(local_index))); | ||
| let label = ValueLabel::from_u32(local_index); | ||
| builder.set_val_label(state.peek1(), label); |
There was a problem hiding this comment.
Instead of doing peek1 here to pull the Value back off the stack, can you just save the return value from use_var and use that instead? And ditto for TeeLocal below.
Also, if we switch from ValueLabel to just using Variable per my comment above, we could actually move this code into FunctionBuilder::use_var and def_var, which would make this functionality available to non-wasm users of Cranelift as well.
Also, I'm concerned about the overhead of doing set_val_label, which can do HashMap and Vec insertions, since GetLocal and SetLocal are some of the most frequent wasm opcodes. Do you know if this is significant right now? If it is, and if we make the other changes, one option would be to give FunctionBuilder a flag indicating whether it should collect debug info or not.
There was a problem hiding this comment.
yes, I can add a flag to not collect this info
| @@ -127,6 +128,12 @@ fn declare_wasm_parameters(builder: &mut FunctionBuilder, entry_block: Ebb) -> u | |||
| let param_value = builder.ebb_params(entry_block)[i]; | |||
| builder.def_var(local, param_value); | |||
| } | |||
| if param_type.purpose == ir::ArgumentPurpose::VMContext { | |||
| // Tracking vmctx as 0xffff_fffe label. | |||
| const VMCTX_LABEL: u32 = 0xffff_fffe; | |||
There was a problem hiding this comment.
Ah, this may complicate my suggestion above. Would it be possible to make cranelift-wasm declare an extra variable for this? It knows how many locals it has, so it can use that to compute the index one greater than the greatest local. That would also eliminate a magic-number dependency.
|
Unfortunately, a lot of tests are now failing with "thread 'worker #0' panicked at 'Code layout must be computed first', cranelift-codegen/src/ir/function.rs:190:9". Could you take a look? |
yurydelendik
force-pushed
the
get-val-labels
branch
3 times, most recently
from
ad8e48c to
2e193c0
Compare
yurydelendik
force-pushed
the
get-val-labels
branch
from
2e193c0 to
96f865b
Compare
|
Looks good, thanks! |
ValueLabelto every values that store the (wasm) variable valueTODO:
build_value_labels_rangesalgorithmic complexityfix range to start from top of EBBExample:
https://gist.github.com/yurydelendik/46e12a65afc63d866b8482915dfe286b#file-fib2-wasm