We must split dbg.assign intrinsics into fragments similarly to what SROA already does for dbg.declares, except that there's many more intrinsics to split. The function migrateDebugInfo generates new dbg.assigns intrinsic for each part of a split store.
Details
- Reviewers
jmorse - Commits
- rGf354716b052d: Reapply [Assignment Tracking][13/*] Account for assignment tracking in SROA
rG3bfba672afd5: [Assignment Tracking][13/*] Account for assignment tracking in SROA
rG5e0b29bf232e: Revert "[Assignment Tracking][SROA] Follow-up for failing test"
rG9517806064cf: Revert "[Assignment Tracking][13/*] Account for assignment tracking in SROA"
rG285d46ef4b60: [Assignment Tracking][SROA] Follow-up for failing test
rGe16d59973ffe: [Assignment Tracking][13/*] Account for assignment tracking in SROA
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
122–123 | Things like IRBuilderBase::CreateExtractInteger assume an 8 bit byte, so I think we're probably fine to assume it here too. | |
134–136 | 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. | |
180 | Style preference: separate line to call this lambda on. | |
191 | llvm_unreachable is probably more cannonical. Or, assigning the Optional E, asserting it, then dereferencing it. Seems like un-necessary control flow otherwise. | |
205 | ||
216 | ||
217 | We should be able to identify difficulties / complications from this in our end-to-end Dexter tests, so I'm not too concerned. | |
2875–2878 | IMO too verbose -- "Capture V for the purpose of debug-info accounting once it's converted to a vector store". |
+ 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.
llvm/lib/Transforms/Scalar/SROA.cpp | ||
---|---|---|
191 | 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? | |
205 | const incorrectness |
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.
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 | ||
---|---|---|
203 | I believe this is always true, probably because of some refactors. | |
3227 | 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. |
llvm/lib/Transforms/Scalar/SROA.cpp | ||
---|---|---|
203 | It's initialized to nullptr outside of this loop. | |
3227 | 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). |
llvm/lib/Transforms/Scalar/SROA.cpp | ||
---|---|---|
203 | Ah, missed the outer loop. |
+ 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
llvm/lib/Transforms/Scalar/SROA.cpp | ||
---|---|---|
3227 | Still working on this test - no luck so far. |
+ Added a test case for the adjusted-pointer update: memmove-to-from-same-alloca.ll
llvm/lib/Transforms/Scalar/SROA.cpp | ||
---|---|---|
3227 | memmove-to-from-same-alloca.ll |
LGTM, note you've added a 'tmp.ll' that you probably didn't intend to? Best to remove that.
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.
+ Fold-in previous test fix 285d46ef4b60c0919c00661199c1b010996cc2c1
+ Fix use-after-free (use at::deleteAssignmentMarkers rather than eraseFromParent in a at::getAssignmentMarkers loop).
Things like IRBuilderBase::CreateExtractInteger assume an 8 bit byte, so I think we're probably fine to assume it here too.