Skip to content
This repository has been archived by the owner on Oct 4, 2022. It is now read-only.

Add orig_insn_map: mapping from original to new insn indices. #50

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Apr 21, 2020

This is needed in the Cranelift client code in order to track
source-location mapping for debug info: we need to be able to shuffle
the source locations into the new locations after the regalloc has done
its instruction-stream editing. The target_map result is not quite
good enough, because it only provides old --> new mappings at a basic
block granularity.

@julian-seward1
Copy link
Contributor

I see why this would be wanted. But it's another 8 bytes per instruction that the allocator needs to sit on and then drag through the machine's caches, and we're already way slow. Is this mapping needed in production runs? If not, I'd prefer to have it implemented similar to the existing request_block_annotations facility -- that is, the client passes in a flag requesting the mapping, and we return it as an Option<TypedIxVec<..>> instead. That way we can have our cake and eat it, albeit at a little more implementation complexity.

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Had a few comments below, in addition to Julian's remark.

@@ -463,15 +471,18 @@ fn add_spills_reloads_and_moves<F: Function>(
&& insts_to_add[curITA].point == InstPoint::new_reload(iix)
{
insns.push(insts_to_add[curITA].inst.construct(func));
orig_insn_map.push(None);
Copy link
Member

Choose a reason for hiding this comment

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

I suppose you maintain a mapping of new instructions to old instructions, thus needing to push None; could it also work with the inverted mapping (old instructions to new instructions), removing the need for None elements in the array?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would make debuginfo generation slower, as you would need to iterate through the whole map to find the original instruction corresponding to the instruction you are currently emitting lineinfo for. Also you may want to attribute those extra insts to iix.

Copy link
Member

Choose a reason for hiding this comment

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

Good point about attributing those extra insts to iix.

However i think there's still a linear way to find the source information for a given new inst, since the source locations are handled all at once: start with the first old instruction, remember what its new instruction is, then on the second instruction, you see the new inst mapping to the second instruction; you then know that all the instructions between first's new instruction and second's previous new instruction all map to the first old instruction.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnjbvr I didn't fully grok your last sentence, but I think you're more-or-less suggesting to invert the map in one linear pass?

I actually originally had the regalloc side of this do exactly as you say, but the issue is then we have to allocate a O(|new_insns|)-sized array anyway to do the inversion, and we have the extra loop with random memory accesses to do it.

IMHO with the SourceLoc::default() value you had mentioned in the other PR, this cost (4 bytes per new-insn) is a pretty reasonable tradeoff...

lib/src/inst_stream.rs Outdated Show resolved Hide resolved
@cfallin
Copy link
Member Author

cfallin commented Apr 22, 2020

@julian-seward1 re: only doing this when necessary: it appears that the SourceLoc info is always needed, as shown for example by the panic caused by code we had to disable to get wasmtime to run at all. It's possible we could push the notion of "source locations are optional" further up the stack, but so far our clients seem to be built assuming it's always there.

Per @bnjbvr in the related Cranelift PR, we have SourceLoc::default() so it's actually just 4 bytes per instruction -- not ideal, but IMHO not the end of the world either, relative to our core algorithms' costs...

@cfallin
Copy link
Member Author

cfallin commented Apr 22, 2020

Err, I misspoke w.r.t. the default val (we're in the regalloc crate here -- we're operating with InstIx's); perhaps we could have a "no instruction" special value in the typed index definitions to allow for this savings?

@julian-seward1
Copy link
Contributor

@cfallin ah well, so be it. If we have to have it, we have to have it. I'll try and quantify the cost in the next round of RA profiling.

@cfallin
Copy link
Member Author

cfallin commented Apr 22, 2020

Just added a ::not_present() option to index types (turns out there were several ad-hoc 0xFFFFFFFF not-present literals already); PTAL!

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

LGTM. The fact the name not_present leaks in a few places where a default impossible value was used is a bit unfortunate, but the alternative doesn't seem much better (i.e. change vocabulary around this to "set/unset" or "defined/undefined" or "default value vs non-default", and then the regalloc.rs users may be misled by its name). Shipit!

lib/src/data_structures.rs Outdated Show resolved Hide resolved
This is needed in the Cranelift client code in order to track
source-location mapping for debug info: we need to be able to shuffle
the source locations into the new locations after the regalloc has done
its instruction-stream editing. The `target_map` result is not quite
good enough, because it only provides old --> new mappings at a basic
block granularity.

This commit also adds "invalid" values to the index types; this is
useful to represent "new instruction maps to no old instruction" without
the memory-layout waste of an `Option<u32>`.
@cfallin cfallin merged commit b349cfa into bytecodealliance:master Apr 24, 2020
@cfallin cfallin deleted the orig-insn-map branch July 16, 2020 18:37
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.

4 participants