This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking Analysis][1/*] Add analysis pass core
ClosedPublic

Authored by Orlando on Oct 20 2022, 1:04 AM.

Details

Summary

This patch stack implements the assignment tracking analysis. This patch contains the main body, but there are unfortunately a few more large code blobs to follow.

The problem and goal

Using the Assignment Tracking "model" it's not possible to determine a variable location just by looking at a debug intrinsic in isolation. Instructions without any metadata can change the location of a variable. The meaning of dbg.assign intrinsics changes depending on whether there are linked instructions, and where they are relative to those instructions. So we need to analyse the IR and convert the embedded information into a form that SelectionDAG can consume to produce debug variable locations in MIR.

The core of the solution is a dataflow analysis which, aiming to maximise the memory location coverage for variables, outputs a mapping of instruction positions to variable location definitions.

High level overview and API

AssignmentTrackingAnalysis is a pass that analyses IR to produce a mapping of instruction positions to variable location definitions. The results are encapsulated by the FunctionVarLocs class.

The pass is integrated with LLVM in this patch but the analysis is not used yet. A future patch updates SelectionDAG separately.

The results of the analysis are exposed via getResults using the returned const FunctionVarLocs *'s const methods:

const VarLocInfo *single_locs_begin() const;
const VarLocInfo *single_locs_end() const;
const VarLocInfo *locs_begin(const Instruction *Before) const;
const VarLocInfo *locs_end(const Instruction *Before) const;
void print(raw_ostream &OS, const Function &Fn) const;

Debug intrinsics can be ignored after running the analysis. Instead, variable location definitions that occur between an instruction Inst and its predecessor (or block start) can be found by looping over the range:

locs_begin(Inst), locs_end(Inst)

Similarly, variables with a memory location that is valid for their lifetime can be iterated over using the range:

single_locs_begin(Inst), single_locs_end(Inst)

Dataflow high level details

The analysis itself is a standard fixed point dataflow algorithm that traverses the CFG using a worklist that is initialised with every block in reverse post order. It computes a result for each visited block that is used to compute the result of successor blocks. Each time the result changes for a block its successors are added to the worklist if not already present. The analysis terminates when the result of every block is stable. Care has been taken to ensure that the merging of information from predecessor blocks yields a result that changes monotonically.

For each block we track "live-in" (LiveIn) and "live-out" (LiveOut) results. The former represents the currently known input to a block, which is the merged (join) result of the live-outs of visited predecessors (empty for the entry block). The live-in set is copied to create a working set for the block (LiveSet). The working set is modified as each instruction in the block is processed (process). After processing the last instruction in the block, the working set is the live-out result for the block. The "results" are BlockInfo objects. These encode assignments to memory and to variables, and track whether each variable's memory location is a good debug location for the variable or not. The actual variable location information (concrete implicit location value, or memory address) is stored off to the side in InsertBeforeMap, which is used after the dataflow is complete to build the instruction -> location definition mapping.

Patch tour

Here's a high-level call-graph that hopefully helps patch navigation.

+-runOnFunction
  +-analyzeFunction
    +-run
      +-process
      | +-processNonDbgInstruction
      | | +-processTaggedInstruction
      | | +-processUntaggedInstruction
      | |  
      | +-processDbgInstruction
      |   +-processDbgAssign
      |   +-processDbgValue
      |   
      +-join
        +-joinBlockInfo
          +-joinLocMap
          | +-joinKind
          |
          +-joinAssignmentMap
            +-joinAssignment

AssignmentTrackingLowering::run (just run above) is where the dataflow starts. Most of this function is dedicated to initialize helper structures and setup worklist traversal scaffolding. The important functions called from here are join and process.

It's probably easier to start with join as it will result in an understanding of the types involved, giving process more meaning. join is responsible for merging the live-outs of predecessors. See the docu-comment at the forward declaration in the class definition. join calls other joinXYZ methods and those call another set, working on merging every element of BlockInfo.

BlockInfo is made up of 3 maps.

LocMap LiveLoc;
AssignmentMap StackHomeValue;
AssignmentMap DebugValue;

LiveLoc maps variables to LocKind, which describes the current kind of location for each variable.
StackHomeValue maps variables the last Assignment to its stack slot (N.B. looking at this now, maybe it should be keyed by address rather than variable - this can come later as a refactor if necessary as it will likely need changing with one of the TODO list items (in D132220)).
DebugValue maps variables to the last Assignment to the variable.

process is where instructions in a block are analysed. The important functions here are addMemDef, addDbgDef, setLocKind, and emitDbgValue. All the leaf process functions call these so I didn't add them to the call graph map. A call to addMemDef states a store with a given ID to a variable fragments's memory location has occurred . Similarly, addDbgDef states an assignment with an ID to a fragment of a variable has occurred. When the variable's memory location assignment and the debug assignment "match" a variable location definition describing the memory location is emitted. Otherwise, an appropriate implicit location value is chosen. setLocKind sets whether the current variable location for the variable is Mem, Val or None and emitDbgValue saves the location to InsertBeforeMap.

The analysis tracks locations for each fragment of each variable that has a definition (/is used in a debug intrinsic). addMemDef, addDbgDef, and setLocKind apply their changes to all fragments contained fully within the one passed in. So, an assignment to bits [0, 64) of a variable is noted for bits [0, 32) too.

I'm aware this patch is large and that tour is not. Hopefully it gives reviewers a good starting point though. Please don't hesitate to ask questions!


Tests are coming in a separate patch.

Diff Detail

Event Timeline

Orlando created this revision.Oct 20 2022, 1:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 1:04 AM
Orlando requested review of this revision.Oct 20 2022, 1:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 1:04 AM

Please avoid using UndefValue::get as much as possible as we are trying to get rid of undef. Please use PoisonValue whenever possible.
Thank you!

Still reviewing, with possibly higher-level comments coming, but just putting forward some of the smaller stuff now.

llvm/include/llvm/CodeGen/AssignmentTrackingAnalysis.h
26
llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
58–59

I think the main value is in having FunctionVarLocs be a read-only interface, which I think makes the split worthwhile.

64
178

Could be a little more verbose.

189–190

Is there a reason this couldn't be inserted with .insert()? The for-loop with emplace_back would only be necessary if Variables and Builder->Variables were different types I think, but I might be missing something.

276–280

Does this behaviour need to be performed by operator==? I see it's used explicitly in hasVarWithValue to determine whether a valid stack home location exists for a variable, and also in equ to compare AssignmentMaps, but before looking for the definition for this operator I was a bit confused as to when that function would ever return true, since the Source field here would be different between DbgDefs and MemDefs. It's a bit of a surprise that operator== doesn't compare all of the fields in this class, especially since this is close to being a POD-type; if this isn't needed to make some template class functions work, could this be changed to be a distinct function isJoinableWith (or another name if you see fit)?

310–314

Minor typo. Also, is there a reason that the argument assigned to ID is called Value? Asking more to actually understand the name assuming that there is a reason, although if there isn't any particular reason then I'd suggest changing the name.

312
331

Slight rename request; either this or anything along these lines.

354
536–539

Comment seems out-of-date w.r.t. the arguments; would it be more accurate as "Return true if Var has an assignment in M equal to AV." or something similar?
Also, the name feels slightly misleading here, as "Value" might be reasonably interpreted as referring to a Value, whereas this function only cares about the Status and DIAssignID; seems similar to the use of Value in the Assignment constructor above.

592

Nit, but I think you can just use the boolean value of an optional for this?

678–680

Not sure I understand the first part of this comment, "Use an assignment ID that nothing can match against" - is this just referring to the fact that there is no DIAssignID, so makeNoneOrPhi must be used to create the Assignment? Also, minor typo.

693–694
733

Could do with a shortened version of the comment as an assert message here.

762
975

This check looks identical to the definition of Assignment::operator!=, could use that instead (though I think it should also be a distinct function instead of an operator, see comment above).

Many thanks for taking this review on. I've answered some of your questions in line.

llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
189–190

They are indeed different types - Variables is a SmallVector and Builder->Variables is a UniqueVector (I figured there was no need in paying the additional memory overhead cost of a UniqueVector since we're done inserting elements at this point.

276–280

SGTM

310–314

Hmmm, no reason that I can remember. I think it's probably just a prototype-hangover - I'll change it.

536–539

Yep you are right there. Agree that the name is misleading, I'll change that.

678–680

is this just referring to the fact that there is no DIAssignID, so makeNoneOrPhi must be used to create the Assignment

Pretty much. By "nothing can match against [it]" I meant hasVarWithValue will return false now for Known AssignmentValues. I am not sure this extra commentary is necessary though - I think I'll cut it down.

General implementation of the patch LGTM!

llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
189–190

Sorry, I meant the element types of the vectors - even for different vector types, I believe Variables.insert(Variables.begin(), Builder->Variables.begin(), Builder->Variables.end()); should work - if not, then please ignore!

664

Just confirming, will this loop body be executed multiple times for the same variable if the base alloca is linked to multiple DbgAssignIntrinsics? I don't think it would result in any incorrectness if that is the case, since this loop body is idempotent, but it might slow things down a little and possibly spam dbgs() a bit.

1236

Could remove the !Pending.empty() condition, since Pending is empty when we reach this loop and is asserted to be empty at the end of each iteration?

1290–1292

Nit, shadowed variables here and in the "Insert the other DEFs" loop below (could use structured bindings?)

1320

Nit: Normally not too bothered about having many asserts even when they seem obvious, but assert(Simple) probably isn't needed just a few statements down from if (!Simple) { ...; continue; }

1397
Orlando planned changes to this revision.Nov 15 2022, 2:03 AM
Orlando updated this revision to Diff 475764.Nov 16 2022, 4:02 AM
Orlando marked 21 inline comments as done.

Please avoid using UndefValue::get as much as possible as we are trying to get rid of undef. Please use PoisonValue whenever possible.
Thank you!

Hi @nlopes. This patch is part of the series in which we agreed this could be addressed after landing the stack: https://reviews.llvm.org/D133293#inline-1286892. That is the next item on my TODO list and I plan to make the change for all debug info modes (rather than just this new one). Is that still okay with you?


Thanks @StephenTozer, I've made those changes.

llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
189–190

Aha, gotcha, thanks. Changed to use append.

664

Yeah it will. I think that having multiple identical dbg.assigns linked to an alloca is unlikely though - IMO it's not worth filtering them out.

1236

True, nice catch.

1290–1292

could use structured bindings

Not for the outer loop as first isn't used & causes an unused variable warning. Will do for the second (this code was written before the move to C++17!).

1320

I've added a comment - if you still think it should go then I'll remove it.

Please avoid using UndefValue::get as much as possible as we are trying to get rid of undef. Please use PoisonValue whenever possible.
Thank you!

Hi @nlopes. This patch is part of the series in which we agreed this could be addressed after landing the stack: https://reviews.llvm.org/D133293#inline-1286892. That is the next item on my TODO list and I plan to make the change for all debug info modes (rather than just this new one). Is that still okay with you?

Ah, yes, sorry, my spam bot is very indiscriminate..

jmorse added a subscriber: jmorse.Nov 20 2022, 12:50 PM

I'm in as far as line 900, hitting submit to clear the queue of comments just in case I don't get back to this. Random remarks:

NB: the locs_begin and single_locs_begin ranges could also have a locs() and single_locs() methods that return iterator_range objects made with make_range, that allows you to use range-based for loops. On the other hand, there's only two places in this patch that they're used, so it might not be a big deal.

Using the term "location" everywhere initially made me twitch as in the IR we have Values... but then again the whole point of assignment tracking is that the location can sometimes be the stack, where the Value isn't know.

Why does NotAlwaysStackHomed ignore fragments -- can't SROA cut up variables into parts, some of which could be partially promoted and mutated, others of which aren't? Perhaps it's an approximation where any partially promoted variable gets explicitly tracked for all fields (this seems fine).

llvm/include/llvm/CodeGen/AssignmentTrackingAnalysis.h
2–3

meganit, s/LIB/INCLUDE/, or something

9–12

Is this convention, to close and open the namespace each time?

34
36
39–42

Shouldn't the comment wording be inverted -- it maps a position to a range, no?

56–58

I'd suggest returning a (const?) reference unless there's a real need to return a temporary.

66

I recommend std::advance to do exactly the same thing, but making it very clear to the reader that you're messing with iterators, rather than having to consider that there's pointer arithmetic happening. Here and elsewhere.

llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
2

IIRC clang-format has an opinion on the order of includes

78–80

Return a const reference instead?

84

Recommend returning a SmallVectorImpl pointer, which I think is a superclass of SmallVector. There's no practical difference but it avoids implicitly encoding the size of the vector in the method information.

(I get the feeling this wants to be std::optional too, however you can't have optional references iirc, so it's probably a bad idea).

91–95

Given the usage of this method, consider an rvalue / std::moveable method too, that might save some un-necessary memory allocations.

105

I want to say "use emplace_back", but I can't imagine it makes much difference. Up to you.

165

Reference argument instead of pointer? It's unconditionally dereferenced.

176

auto & or you'll generate a temporary, I think.

180
205

Passing in a reference to a pointer might be slightly neater -- depends whether the explicit dereferencing of Expression is designed to signal to the reader that there's a side-effect. An alternative would be returning a {Value,Expression} pair. This is slightly stylistic, up to you.

212–213

This prepending could be put inside the conditional, yes?

238–240

This doesn't seem to be how joinKind implements it; additionally if we can switch from {Mem,Val} to None, and then {None,Mem} to Mem, doesn't that mean there can be a "Val" in a predecessor block but this isn't reflected in the lattice value.

245

I'm not sure -> it's not clear, to avoid the reader questioning "who?"

268–273

IMO: too much detail about the algorithm for just a field, better to have that detail in a function, and just the first line documenting the field.

312

IMHO, simpler to read if it's Status == NoneOrPHI || ID, YMMV.

325–326

A map of maps sounds expensive -- do both keys of this really need to be randomly accessed, or could they instead be inserted and sorted at a later date? (I haven't read far down enough to get a grip of how the container is used.

361

This is another one of those places where I feel the word "dominance frontier" can be inserted to a) make ourselves feel clever, and b) actually disambiguate what's going on. i.e., "NoneOrPhi indicates whether there is a single dominating definition which can be found in StackHomeValue or DebugValue". Or something like that? (Might not be right).

384–385

These translate to maps of maps again, which risks an expensive reallocation. There isn't necessarily anything to do about this if that's not triggered by the workloads that actaully exist.

jmorse added inline comments.Nov 20 2022, 12:50 PM
llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
393

const & argument?

414–415

This wants explaining why -- it's because the top value represents "don't know", right?

426–427

If it's initialized, pass by reference instead?

435

and below

444

As ever, default to passing the BlockInfo by reference, and AV as const reference, just for simplicity and to avoid un-necessary locals? And below.

475

Var itself is a fragment, right? Small risk that the reader thinks it's a DILocalVariable? (Our terminology doesn't help).

493–494

This slightly threw me; the "None" value being set isn't important because everything that calls addMemDef calls setLocKind too, right? If so, best to document this in a comment please.

565–568

If it's a scenario that llvm will never generate nowadays, and the test is testing something unrelated, might be easier to fix the test rather than make the actual code suffer for the past.

571

Hhrrrrmmmm. Using getNextNode makes me twitch on account of it being the source of various debug-affects-codegen problems in the past, or hard-to-unpick debug behaviours. In this context it's certainly the right thing to use though (fry-eyes.jpg)

635

This is all fine; IMO there needs to be a moderately detailed comment about what this is doing at the overall-algorithm level, i.e. "interpret stack stores that are not tagged as an assignment in memory because [blah]". I remember this from past discussions, IMO it should be stated at the outset what this method is aiming to do.

648

Is this cast necessary? Might be better to put a trailing type on the lambda to make it un-necessary? Any particular need for a lambda instead of something else?

679
716

This looks good; I suspect I'd find tests highly valuable in this situation to understand exactly the behaviour that's being created, I understand they're in a later patch though.

751

early-continue here will save a level of indentation for the rest of the loop.

825

It looks slightly out of place that the LocKind is recorded as a Val, but then emitDbgValue is used with LocKind::Mem -- I suppose the problem is that I don't know what's meant by elision of the original store. What are the effects of this scenario {LocKind==Val,emitDbgValue(Mem...)}?

838

\n

847–849

Would I be right in thinking that these dbg.values turn up because of promotion, and thus the dbg.values are "normally" attached to PHIs? If so, is it possible to assert that fact. If not maybe the comment could be reworded to give the impression that this isn't an omission in the design, it's the natural effect of the design. (I think the term "we must..." makes me feel like it's a defect).

jmorse added inline comments.Nov 20 2022, 1:27 PM
llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
916

Note to self, returning a DenseMap by value is probably fine if it gets NRVO'd.

933

auto & or you might get storage.

976

Coming immediately after the leading comment, shouldn't this also check if A.Status == NoneOrPhi?

1088

/me squints -- I want to say that BBLiveIn being the argument and the return value will lead to some kind of aliasing weirdness or performance slowdown (in the form of un-necessary DenseMap copies). I can't actually put my finger on why that would be forced though. Do you have any feeling for it?

1192
jmorse requested changes to this revision.Nov 21 2022, 3:42 AM

(Ticking "request changes" to take it out of the review queue)

llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
1329

auto &, don't want to risk copying the mapvector

This revision now requires changes to proceed.Nov 21 2022, 3:42 AM
Orlando updated this revision to Diff 477229.Nov 22 2022, 9:25 AM
Orlando marked 40 inline comments as done.

Ah, yes, sorry, my spam bot is very indiscriminate..

Great, thanks! & no worries.


Thanks @jmorse for the detailed review - that should be all the inline comments addressed (or responded-to) but please do ping if I missed any.

NB: the locs_begin and single_locs_begin ranges could also have a locs() and single_locs() methods that return iterator_range objects made with make_range, that allows you to use range-based for loops. On the other hand, there's only two places in this patch that they're used, so it might not be a big deal.

Initially I opted to avoid adding more code, happy to change that if you prefer.

Using the term "location" everywhere initially made me twitch as in the IR we have Values... but then again the whole point of assignment tracking is that the location can sometimes be the stack, where the Value isn't know.

Why does NotAlwaysStackHomed ignore fragments -- can't SROA cut up variables into parts, some of which could be partially promoted and mutated, others of which aren't? Perhaps it's an approximation where any partially promoted variable gets explicitly tracked for all fields (this seems fine).

Yeah it is an approximation - we only consider variables that are wholly located in one alloca as ones to put in the MachineFunction stacked-homed-variable side table. SROAd variables that you described above will fall back to using location lists (which may include stack locations).

llvm/include/llvm/CodeGen/AssignmentTrackingAnalysis.h
2–3

Changed to copy other headers in include/CodeGen.

9–12

Ah, nope, that was include-what-you-use. Fixed.

39–42

It made sense to me but happy to change it.

llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
2

clang-format was happy with (/reinstated) this order (perhaps xxx.h is allowed at the top of xxx.cpp).

91–95

I think the next patches in the stack have similar usage so I've just removed the copying version.

205

signal to the reader that there's a side-effect

That was the intention, but I prefer yout pair idea. Done.

238–240

This comment is wrong / out of date, sorry. Fixed.

268–273

Moved the comment body into joinAssignment.

325–326

The maps are { instruction that comes after : { variable frag instance : location definition }. The location definitions are added to the map each time they're calculated. Huh - I think this needs to clear the sub-map each before each instruction is visited. And after doing that, I can see that we don't need the sub-map at all. Replaced and added resetInsertionPoint. Good spot, thanks.

361

That code comment is indeed confusing at best. I've had a go at rewording it, let me know what you think. Happy to make further changes.

384–385

I'm not sure that these ones can be avoided. These are at least initialized to a size equal to the number of basic blocks, down in AssignmentTrackingLowering::run.

414–415

I've elaborated a bit. Is this ok or does it need more?

426–427

I chose to use a pointer to hint at call sites that it is modified. If that is not compelling (or the hint is not doing its job) I can change it - just thought I'd offer up my rationale first in case it made a difference.

444

(process reply applies - made the const& change though)

475

Yes (if by "is a fragment" you mean is an ID that identifies a {DILocalVariable, FragmentInfo, InlinedAt} tuple aka a DebugVariable, which indeed includes fragment information). I don't think I've deviated massively from common naming practices but I agree that our terminology isn't necessarily helpful here. DebugVariable should probably be called something like VariableInstanceFragment (but that is outside the scope of this patch of course). Do you have any suggestions for Var / VariableID?

493–494

Yeah that's right, in this instance we're just adding Var to LiveSet if it isn't there already (using insert). I'll update the comment

565–568

It still happens - it's what is returned in by getVariableLocationOp when the wrapped Value is replaced with empty metadata.

635

I've put that here but is there somewhere better for it should live / be copied to, do you think? (& what do you think of the comment?).

648

Ah, AssignmentInfo has no default ctor so we can't do this:

AssignmentInfo Info;
if ...
    Info = ....
else if ...
     Info = ...

Let me know if you'd prefer that though as I can just add one.

825

It looks slightly out of place that the LocKind is recorded as a Val, but then emitDbgValue is used with LocKind::Mem

It does look slightly out of place. This could be a bug - I'll get back to you on this one.

847–849

They should mostly be attached to PHIs, but can sneak in in other ways too. I'll reword it.

916

That's what I'm counting on, but I'm happy to change it if it's unconventional / looks suspicious.

933

Good point - and changed to use structured binding for the pair while I'm here (and in`joinAssignmentMap`).

976

That is checked right here above your review comment. I used separate ifs for readability, but happy to combine them if this was counterproductive?

1088

Hmmm, I don't remember this sticking out in the profiles but it was a while ago that I looked at the performance. I have wrapped the argument in a std::move, which could help.

Looks good with some nits fixed, a spurious return removed, and the question on line 825 addressed.

Of course, landing this would require some tests; I believe you'll get stern looks if this lands with nothing covering it. Thus you might want to fold in the tests from patch 5 as appropriate, when they're reviewed.

llvm/include/llvm/CodeGen/AssignmentTrackingAnalysis.h
66

(done below, but here appreciated too, not a blocker)

llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
165

While we're at it, const auto &

361

Much more understandable now, cheers!

384–385

Hrrrmmm. I suppose over in LiveDebugValues we have exactly this, but with a SmallVector of DenseMaps where the block number is the index. That's pretty much identical to LiveIn / LiveOut here. Given this is a dataflow algorithm, random access to blocks is unavoidable, and within those random access to variables, which requires two levels of map.

That being said, I eventually found it faster to deal with a single variable at a time and trade one locality with another. But again, we should only re-visit this if it turns out that there's a performance loss.

414–415

SGTM

426–427

(I think this was applied to process before the comments floated) This is moderately a style thing, so up to you in the end.

475

Nah, just making sure I understood correctly.

648

No need, a potentially un-necessary lambda is better than opening the scope of "bugs we can introduce by allowing default-constructed data structures".

656
825

(This space intentionally left blank -- to signal to myself that there'll be a response coming back from you at some point).

898

Reference rather than pointer given it's unconditionally dereferenced?

900

Leftover return from prototyping?

976

Nah, I'm just blind apparently

Orlando updated this revision to Diff 477885.Nov 25 2022, 1:30 AM
Orlando marked 19 inline comments as done.

That should be all comments addressed

llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
384–385

SGTM

825

Updated this (use same LocKind for setLocKind / emitDbgValue here). I am not sure that we need the undef check at all though as this pattern is no longer generated (at least not on purpose). This change results in a DBG_VALUE $noreg to lose DW_OP_deref from its expression in mem-loc-frag-fill.ll in the test patch.

Of course, landing this would require some tests; I believe you'll get stern looks if this lands with nothing covering it. Thus you might want to fold in the tests from patch 5 as appropriate, when they're reviewed.

My original plan / hope was to land these analysis patches and tests contiguously. If that isn't agreeable then I can try to fold some in, though it'll still require either squashing or contiguous patch landing because this patch requires D136331 to land before it can actually be tested.

Orlando updated this revision to Diff 479276.Dec 1 2022, 7:13 AM

In improving test coverage in D136335 I discovered a bug: the fragments that untagged stores affect were not added to the OverlapMap` (map of fragment to fragments that are contained within). This meant that fragments were not always correctly clobbered upon visiting an untagged store. I've fixed this by adding yet more analysis setup code to buildOverlapMapAndRecordDeclares. This function now also interprets untagged stores to add the fragments to the OverlapMap and populates UntaggedStoreVars, caching the assignment info (offset, size, base alloca, variable fragment) for lookup during the analysis.

Orlando updated this revision to Diff 479323.Dec 1 2022, 8:56 AM
Orlando updated this revision to Diff 480875.Dec 7 2022, 5:51 AM

Fix another bug to do with fragments that was found with a new test in D136335. In short: we weren't checking for matching assignments to fragments contained within a fragment, so a def of a larger fragment would still be considered live after a def to a contained fragment (if the larger fragment assignment was queried only).

jmorse accepted this revision.Dec 9 2022, 6:58 AM

Latest two revisions look good -- except that you've not merged them together when uploading them, so the latter overwrite the former!

Overall LGTM -- as discussed offline, it's awkward that these five patches all depend on each other. We should try for landing the lot together (and seeing what magnitude of breakage the buildbots discover) and then possibly decompose the merged patch down from there.

This revision is now accepted and ready to land.Dec 9 2022, 6:58 AM
Orlando updated this revision to Diff 481647.Dec 9 2022, 7:21 AM

Latest two revisions look good -- except that you've not merged them together when uploading them, so the latter overwrite the former!

Ah, nice catch - I'd re-ordered my patches in a rebase and then missed the first patch (reordered to top of stack) from the second update (and accidentally dragged in other changes too). Sorry about that! That should be fixed now.

Orlando updated this revision to Diff 481650.Dec 9 2022, 7:34 AM