cranelift: Turn AliasRegion into an entity stored in the DFG#13354
cranelift: Turn AliasRegion into an entity stored in the DFG#13354fitzgen wants to merge 1 commit into
AliasRegion into an entity stored in the DFG#13354Conversation
fitzgen
force-pushed
the
alias-region-entity
branch
from
b094d0f to
29b3c7d
Compare
github-actions
Bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:meta", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks! Some thoughts below but overall shape looks good. Happy this worked out w.r.t. compile-time overheads!
| // We need to mutate `self` if this instruction accepts a value list, will construct | ||
| // BlockCall values, or has memflags operands (which need DFG insertion). | ||
| // We need to mutate `self` if this instruction accepts a value list, will construct | ||
| // BlockCall values, or has memflags operands (which need DFG insertion). |
There was a problem hiding this comment.
Duplicated comment (maybe a merge or rebase issue?)
| // * 3 - big endian flag | ||
| // * 4 - checked flag | ||
| // * 5/6 - alias region | ||
| // * 5/6 - (unused, formerly alias region) |
There was a problem hiding this comment.
we can just say "unused" here I guess -- no need to embed the history of the bits in the comment?
| bits: u16, | ||
|
|
||
| /// The alias region for this memory operation, if any. | ||
| region: Option<AliasRegion>, |
There was a problem hiding this comment.
PackedOption maybe so None can fit in the reserved_value niche? I know this won't be as memory-critical today because it's dedup'd but if we end up with a lot of these it might still matter...
| dedupe_map: HashMap<MemFlagsData, MemFlags>, | ||
| } | ||
|
|
||
| impl Hash for MemFlagsSet { |
There was a problem hiding this comment.
As long as we're manually implementing Hash we could do so for PartialEq/Eq as well for efficiency, since we can skip dedupe_map for that too (it's purely derived data), right?
| /// Insert new mem flags into this set. | ||
| /// | ||
| /// Returns an existing `MemFlags` if the data already exists. | ||
| pub fn insert(&mut self, data: MemFlagsData) -> MemFlags { |
There was a problem hiding this comment.
Should this be the point in the API that's fallible, so we can propagate the u16-index-space allocation failure upward (e.g. to wasmtime-cranelift when it uses these for a user-controlled number of regions)?
| &self.frame_layout | ||
| } | ||
|
|
||
| fn set_mem_flags(&mut self, mem_flags: crate::ir::MemFlagsSet) { |
There was a problem hiding this comment.
This seems like a fairly awkward and inefficient bit of plumbing to get the MemFlagsSet down to the point of emission -- maybe we could pass a borrow into MachInst::emit instead?
Also, separately, it's a somewhat conspicuous layering violation: we have an IR type all the way down in the VCode. I'm not completely categorically opposed to that in all cases, but I wonder if we could lower MemFlagData to a "core" memflags actually semantically meaningful at the VCode layer (so, no alias regions any more, just trap info and the like) and use that in the backends?
| // Store with no memflags: must clobber everything. | ||
| for (_ar, last) in self.regions.iter_mut() { | ||
| *last = inst.into(); | ||
| } |
There was a problem hiding this comment.
Since the last-fence becomes the fallback, could we clear regions here instead?
| ; block1: | ||
| ; xone x0 | ||
| ; f1 = fload64 Slot(0) // flags = notrap aligned | ||
| ; f1 = fload64 Slot(0) // flags =memflags0 |
There was a problem hiding this comment.
here's another example where we might benefit from having a VCode-specific "core memflags": the printed form here is relatively useless (refers to an index of an entity that we don't print anywhere else in the disassembly) whereas ideally we'd print the same as before, after lowering.
Generalize AliasRegion from a fixed enum (Heap/Table/Vmctx) into an entity index with AliasRegionData containing a description string for printing. MemFlagsData grows a region: PackedOption<AliasRegion> field instead of bitpacking the alias region with the rest of its flags. Alias regions are stored in a PrimaryMap<AliasRegion, AliasRegionData> in the DFG. There is no deduplication: each push() creates a new region entity. Users (e.g. Wasmtime) are responsible for caching and reusing their own alias region entities. The CLIF text format is updated to support region declarations in the function header, e.g. region0 = "heap", with memflags referencing regions by entity name instead of the old hardcoded keywords.
fitzgen
force-pushed
the
alias-region-entity
branch
from
29b3c7d to
574b4dd
Compare
|
@cfallin okay I fixed the rebase, cleaned up the region clearing, and introduced |
2 participants
Generalize from
into a proper entity, allowing users to define arbitrary numbers of custom alias
regions.
MemFlagsDatagrows aregion: PackedOption<AliasRegion>field instead ofbitpacking the alias region with the rest of its flags.
The CLIF text format is also updated to support alias region declarations in the
function header, e.g.
region0 = 0 "heap", with memflags referencing regions byentity name instead of the old hardcoded keywords.
Depends on #13353