This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][13/*] Account for assignment tracking in SROA
ClosedPublic

Authored by Orlando on Sep 5 2022, 5:15 AM.

Diff Detail

Event Timeline

Orlando created this revision.Sep 5 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 5:15 AM
Orlando requested review of this revision.Sep 5 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 5:15 AM
jmorse added a subscriber: jmorse.Sep 8 2022, 8:21 AM

As far as I understand it, all the code above splitAlloca is about splitting some alloca that already has dbg.assigns, and creating the appropriate dbg.assigns with an updated fragment. Isn't the additions to splitAlloca doing exactly the same thing? My feeling is that this is something to do with splitting stores being distinct in some way from splitting based on uses?

Note to self: haven't looked at the tests.

The store splitting stuff is tricky because it's all new behaviours -- I don't think that code has been instrumented for debug-info before, meaning we won't find omissions by tests failing. This is unfortunate, but I think it's is necessary for progress to be made.

llvm/lib/Transforms/Scalar/SROA.cpp
116–117

Things like IRBuilderBase::CreateExtractInteger assume an 8 bit byte, so I think we're probably fine to assume it here too.

128–130

Docu-commenting the parameters would be nice -- I'm guessing they're all parts / components of a store, but it's better to be expilcit.

174

Style preference: separate line to call this lambda on.

185

llvm_unreachable is probably more cannonical. Or, assigning the Optional E, asserting it, then dereferencing it. Seems like un-necessary control flow otherwise.

199
210
211

We should be able to identify difficulties / complications from this in our end-to-end Dexter tests, so I'm not too concerned.

2744–2747

IMO too verbose -- "Capture V for the purpose of debug-info accounting once it's converted to a vector store".

jmorse requested changes to this revision.Sep 13 2022, 9:46 AM

(Sending back to "revise" queue given review above)

This revision now requires changes to proceed.Sep 13 2022, 9:46 AM
Orlando updated this revision to Diff 460037.Sep 14 2022, 5:00 AM
Orlando marked 7 inline comments as done.

+ Addressed review comments.

As far as I understand it, all the code above splitAlloca is about splitting some alloca that already has dbg.assigns, and creating the appropriate dbg.assigns with an updated fragment. Isn't the additions to splitAlloca doing exactly the same thing? My feeling is that this is something to do with splitting stores being distinct in some way from splitting based on uses?

The code added to splitAlloca only splits the dbg.assign intrinsics linked to the alloca. Each use of the alloca that is split also needs to update its linked dbg.assigns.

This was one of the first changes made in the prototype - there could well be a better way of approaching this. I will have a think about it. E.g. splitting all the dbg.assign intrinsics at once after SROA has run. I'll mark this as "changes planned" for now and have a little look.

Orlando planned changes to this revision.Sep 14 2022, 5:00 AM
Orlando added inline comments.
llvm/lib/Transforms/Scalar/SROA.cpp
185

I've removed the control flow, but I wonder if keeping both the assert and if is better for release mode in case there is a failure mode I haven't thought of here. I'm not 100% confident that this assert will never fire (in fact, I think it could) - I think it just hasn't fired for the code bases I've built. What do you think?

199

const incorrectness

Orlando requested review of this revision.Sep 15 2022, 7:41 AM

This was one of the first changes made in the prototype - there could well be a better way of approaching this. I will have a think about it. E.g. splitting all the dbg.assign intrinsics at once after SROA has run. I'll mark this as "changes planned" for now and have a little look.

Ultimately I think this would require keeping some kind of record of {pre-split store: split stores} mapping, which is just as liable to miss particular code-paths as the current approach. For now, I think we should stick with what we have. But if we can find an alternative approach in the future I would not be upset.

Orlando updated this revision to Diff 460408.Sep 15 2022, 7:48 AM

Remove accidentally included rubbish .ll file

Looks good, although I think some of the logic at one of the inline comments can be simplified.

Once again I haven't looked in depth at the tests -- clearly they're there to establish good coverage, but each is pretty complicated and it's hard to judge whether all the paths in SROA are covered. I think that's a necessary evil -- this is a new thing, after all, and the tests are necessarily complicated.

llvm/lib/Transforms/Scalar/SROA.cpp
197

I believe this is always true, probably because of some refactors.

3092

First option simplifiable to NewBeginOffset == NewAllocaBeginOffset? It's not immediately obvious what this is checking, and I feel it might be easier to read if we name the other proposition "NoAssignmentMarkers" or something. That might also allow you to condense or invert the control flow. The for loop is dependent on !NoAssignmentMarkers, for example.

Orlando added inline comments.Nov 17 2022, 3:41 AM
llvm/lib/Transforms/Scalar/SROA.cpp
197

It's initialized to nullptr outside of this loop.

3092

I think this blob represents me not being sure what to do here at the time. I don't think the for loop below is correct - based on other similar changes in the patch stack that probably needs to be a replaceUsesOfWith style update. It turns out I didn't have test coverage for this. sitrep: I've now got a test that stimulates this codepath (not uploaded) - but not yet managed to get one that let's us inspect the state at this point (it goes through another round of SROA-ing).

jmorse added inline comments.Nov 17 2022, 5:26 AM
llvm/lib/Transforms/Scalar/SROA.cpp
197

Ah, missed the outer loop.

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

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

This revision now requires changes to proceed.Nov 21 2022, 3:38 AM
Orlando updated this revision to Diff 476853.Nov 21 2022, 4:05 AM

+ Clean up tests (convert to opaque pointers, remove tbaa metadata, use -passes=foo switch)
+ Change "replace" to "replace use of x with y" when updating dest to adjustedPtr in visitMemTransferInst for dbg.assigns

Orlando marked an inline comment as done.Nov 21 2022, 4:06 AM
Orlando added inline comments.
llvm/lib/Transforms/Scalar/SROA.cpp
3092

Still working on this test - no luck so far.

Orlando planned changes to this revision.Nov 21 2022, 4:06 AM
Orlando updated this revision to Diff 477805.Nov 24 2022, 8:19 AM

+ Added a test case for the adjusted-pointer update: memmove-to-from-same-alloca.ll

llvm/lib/Transforms/Scalar/SROA.cpp
3092

memmove-to-from-same-alloca.ll

jmorse accepted this revision.Nov 25 2022, 3:57 AM

LGTM, note you've added a 'tmp.ll' that you probably didn't intend to? Best to remove that.

This revision is now accepted and ready to land.Nov 25 2022, 3:57 AM
This revision was landed with ongoing or failed builds.Nov 28 2022, 3:32 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Nov 28 2022, 4:07 AM

This breaks tests: http://45.33.8.238/linux/92558/step_12.txt

Please take a look and revert for now if it takes a while to fix.

Hi - on it, just testing the fix before pushing.

Orlando reopened this revision.Dec 9 2022, 6:41 AM
This revision is now accepted and ready to land.Dec 9 2022, 6:41 AM
Orlando planned changes to this revision.Dec 9 2022, 6:41 AM
Orlando updated this revision to Diff 482024.Dec 12 2022, 1:18 AM

+ Fold-in previous test fix 285d46ef4b60c0919c00661199c1b010996cc2c1
+ Fix use-after-free (use at::deleteAssignmentMarkers rather than eraseFromParent in a at::getAssignmentMarkers loop).

This revision is now accepted and ready to land.Dec 12 2022, 1:18 AM
This revision was landed with ongoing or failed builds.Dec 12 2022, 1:24 AM
This revision was automatically updated to reflect the committed changes.