Wasmtime GitHub notifications bot (Apr 22 2020 at 01:37): Wasmtime GitHub notifications bot (Apr 22 2020 at 01:37):cfallin opened PR #1575 from test-fixes to master:
This change adds SourceLoc information per instruction in a
VCode<Inst>
container, and keeps this information up-to-date across register allocation
and branch reordering. The information is initially collected during
instruction lowering, eventually collected on the MachSection, and finally
provided to the environment that wraps the codegen crate for wasmtime.This PR is based on top of #1570 and #1571 (part of a series fixing tests).
This PR depends on wasmtime/regalloc.rs#50, a change to the register
allocator to provide instruction-granularity info on the rewritten
instruction stream (rather than block-granularity).With the prior PRs applied as well, quite a few more unit tests pass;
the exclusion list in #1526 should be updated if this PR lands first.
Wasmtime GitHub notifications bot (Apr 22 2020 at 01:37):cfallin requested bnjbvr and julian-seward1 for a review on PR #1575.
Wasmtime GitHub notifications bot (Apr 22 2020 at 01:37):cfallin requested bnjbvr and julian-seward1 for a review on PR #1575.
Wasmtime GitHub notifications bot (Apr 22 2020 at 01:37):cfallin edited PR #1575 from test-fixes to master:
This change adds SourceLoc information per instruction in a
VCode<Inst>
container, and keeps this information up-to-date across register allocation
and branch reordering. The information is initially collected during
instruction lowering, eventually collected on the MachSection, and finally
provided to the environment that wraps the codegen crate for wasmtime.This PR is based on top of #1570 and #1571 (part of a series fixing tests).
This PR depends on bytecodealliance/regalloc.rs#50, a change to the register
allocator to provide instruction-granularity info on the rewritten
instruction stream (rather than block-granularity).With the prior PRs applied as well, quite a few more unit tests pass;
the exclusion list in #1526 should be updated if this PR lands first.
Wasmtime GitHub notifications bot (Apr 22 2020 at 01:38):cfallin edited PR #1575 from test-fixes to master:
This change adds SourceLoc information per instruction in a
VCode<Inst>
container, and keeps this information up-to-date across register allocation
and branch reordering. The information is initially collected during
instruction lowering, eventually collected on the MachSection, and finally
provided to the environment that wraps the codegen crate for wasmtime.This PR is based on top of #1570 and #1571 (part of a series fixing tests).
This PR depends on bytecodealliance/regalloc.rs#50, a change to the register
allocator to provide instruction-granularity info on the rewritten
instruction stream (rather than block-granularity). I have not yet
updatedcranelift/codegen/Cargo.toml; we will need to land that PR
and do a release first.With the prior PRs applied as well, quite a few more unit tests pass;
the exclusion list in #1526 should be updated if this PR lands first.
Wasmtime GitHub notifications bot (Apr 22 2020 at 01:39):cfallin edited PR #1575 from test-fixes to master:
This change adds SourceLoc information per instruction in a
VCode<Inst>
container, and keeps this information up-to-date across register allocation
and branch reordering. The information is initially collected during
instruction lowering, eventually collected on the MachSection, and finally
provided to the environment that wraps the codegen crate for wasmtime.This PR is based on top of #1570 and #1571 (part of a series fixing tests).
This PR depends on bytecodealliance/regalloc.rs#50, a change to the register
allocator to provide instruction-granularity info on the rewritten
instruction stream (rather than block-granularity). I have not yet
updatedcranelift/codegen/Cargo.toml; we will need to land that PR
and do a release first.With the prior PRs applied as well, quite a few more unit tests pass;
the exclusion list in #1526 should be updated if this PR lands first.Addresses part of #1523 (debug info), though debug info consists of
more than just source-location mapping.
Wasmtime GitHub notifications bot (Apr 22 2020 at 08:59):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Apr 22 2020 at 08:59):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Apr 22 2020 at 08:59):bjorn3 created PR Review Comment:
Every function is compiled separately without knowledge of the final location in memory, so this can never be absolute. Did you mean relative tot the start of the function?
Wasmtime GitHub notifications bot (Apr 22 2020 at 08:59):bjorn3 created PR Review Comment:
There is already a SourceLoc reserved for when the source loc is unknown: SourceLoc::default().
Wasmtime GitHub notifications bot (Apr 22 2020 at 13:34):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Apr 22 2020 at 13:34):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Apr 22 2020 at 13:34):bnjbvr created PR Review Comment:
nit: current
Wasmtime GitHub notifications bot (Apr 22 2020 at 13:34):bnjbvr created PR Review Comment:
nit: to remove one extra level of indent, use
expecton the result of take.
Wasmtime GitHub notifications bot (Apr 22 2020 at 13:34):bnjbvr created PR Review Comment:
Could the copy be avoided here? (Either by letting the one client iterate on the sections, or return a Map iterator with
self.sections.iter().map(|section| §ion.srclocs)or something like this)I think this requires sorting the srclocs before we actually sort them, but this is doable.
Wasmtime GitHub notifications bot (Apr 22 2020 at 13:34):bnjbvr created PR Review Comment:
Agreed, it'd be nice to reuse it in lieu of all the
Option<SourceLoc>in this patch.
Wasmtime GitHub notifications bot (Apr 22 2020 at 13:34):bnjbvr created PR Review Comment:
Could
final_srclocs(and thussrclocs) store range of instructions mapping to a single srcloc, instead? (Just the start instruction should be enough, since the next entry's start instruction would be next to the previous' entry end)
Wasmtime GitHub notifications bot (Apr 22 2020 at 13:34):bnjbvr created PR Review Comment:
nit: double space between
theandregion
Wasmtime GitHub notifications bot (Apr 22 2020 at 13:34):bnjbvr created PR Review Comment:
could you debug_assert that
end >= start, for sanity?
Wasmtime GitHub notifications bot (Apr 23 2020 at 00:04):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 00:04):cfallin created PR Review Comment:
I thought about this briefly, but it makes the rearranging after regalloc somewhat more complicated: we have to reconstitute the original per-instruction
srclocs because the block reordering may have split one of the same-location extents in two. I'd rather avoid that complexity (and branchiness), at the cost of slightly more memory... (but perhaps there's another way I've missed).
Wasmtime GitHub notifications bot (Apr 23 2020 at 00:05):cfallin updated PR #1575 from test-fixes to master:
This change adds SourceLoc information per instruction in a
VCode<Inst>
container, and keeps this information up-to-date across register allocation
and branch reordering. The information is initially collected during
instruction lowering, eventually collected on the MachSection, and finally
provided to the environment that wraps the codegen crate for wasmtime.This PR is based on top of #1570 and #1571 (part of a series fixing tests).
This PR depends on bytecodealliance/regalloc.rs#50, a change to the register
allocator to provide instruction-granularity info on the rewritten
instruction stream (rather than block-granularity). I have not yet
updatedcranelift/codegen/Cargo.toml; we will need to land that PR
and do a release first.With the prior PRs applied as well, quite a few more unit tests pass;
the exclusion list in #1526 should be updated if this PR lands first.Addresses part of #1523 (debug info), though debug info consists of
more than just source-location mapping.
Wasmtime GitHub notifications bot (Apr 23 2020 at 00:05):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 00:05):cfallin created PR Review Comment:
Yup, fixed, thanks!
Wasmtime GitHub notifications bot (Apr 23 2020 at 00:06):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 00:06):cfallin created PR Review Comment:
Ah, yes, this is much nicer (as well as the corresponding change to not-present indices on the regalloc side) -- done.
Wasmtime GitHub notifications bot (Apr 23 2020 at 00:08):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 00:08):cfallin created PR Review Comment:
Yup, done; wrote a little custom iterator. (I might have been able to get it to work returning an
impl Iterator<Item = &'a MachSrcLoc>but getting the types to align for the zero-sections case and otherwise is tricky, since impl-returns must match on all returning paths...)
Wasmtime GitHub notifications bot (Apr 23 2020 at 09:27):bnjbvr requested bnjbvr and julian-seward1 for a review on PR #1575.
Wasmtime GitHub notifications bot (Apr 24 2020 at 14:10):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Apr 24 2020 at 20:07):cfallin updated PR #1575 from test-fixes to master:
This change adds SourceLoc information per instruction in a
VCode<Inst>
container, and keeps this information up-to-date across register allocation
and branch reordering. The information is initially collected during
instruction lowering, eventually collected on the MachSection, and finally
provided to the environment that wraps the codegen crate for wasmtime.This PR is based on top of #1570 and #1571 (part of a series fixing tests).
This PR depends on bytecodealliance/regalloc.rs#50, a change to the register
allocator to provide instruction-granularity info on the rewritten
instruction stream (rather than block-granularity). I have not yet
updatedcranelift/codegen/Cargo.toml; we will need to land that PR
and do a release first.With the prior PRs applied as well, quite a few more unit tests pass;
the exclusion list in #1526 should be updated if this PR lands first.Addresses part of #1523 (debug info), though debug info consists of
more than just source-location mapping.
Wasmtime GitHub notifications bot (Apr 24 2020 at 20:18):cfallin updated PR #1575 from test-fixes to master:
This change adds SourceLoc information per instruction in a
VCode<Inst>
container, and keeps this information up-to-date across register allocation
and branch reordering. The information is initially collected during
instruction lowering, eventually collected on the MachSection, and finally
provided to the environment that wraps the codegen crate for wasmtime.This PR is based on top of #1570 and #1571 (part of a series fixing tests).
This PR depends on bytecodealliance/regalloc.rs#50, a change to the register
allocator to provide instruction-granularity info on the rewritten
instruction stream (rather than block-granularity). I have not yet
updatedcranelift/codegen/Cargo.toml; we will need to land that PR
and do a release first.With the prior PRs applied as well, quite a few more unit tests pass;
the exclusion list in #1526 should be updated if this PR lands first.Addresses part of #1523 (debug info), though debug info consists of
more than just source-location mapping.
Wasmtime GitHub notifications bot (Apr 24 2020 at 22:25):cfallin merged PR #1575.
Wasmtime GitHub notifications bot (Apr 25 2020 at 11:35):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Apr 25 2020 at 11:35):bjorn3 created PR Review Comment:
I actually want to know about all instructions, not just those with a non-default srcloc.
Wasmtime GitHub notifications bot (Apr 29 2020 at 18:04):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 29 2020 at 18:04):cfallin created PR Review Comment:
Thanks for pointing this out -- you're right! New PR here: #1632
Last updated: Oct 23 2024 at 20:03 UTC