This is an archive of the discontinued LLVM Phabricator instance.

[mlir][bufferization] Fix bufferization of repetitive regions
ClosedPublic

Authored by springerm on Feb 1 2023, 9:12 AM.

Details

Summary

The previous strategy was too complex and faulty. Op dominance cannot be used to rule out RaW conflicts due to op ordering if the reading op and the conflicting writing op are in a sub repetitive region of the closest enclosing repetitive region of the definition of the read value.

Depends On: D143183

Diff Detail

Event Timeline

springerm created this revision.Feb 1 2023, 9:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 9:12 AM
springerm requested review of this revision.Feb 1 2023, 9:12 AM
chelini accepted this revision.Feb 2 2023, 1:14 AM

Thanks, looks good to me.

This revision is now accepted and ready to land.Feb 2 2023, 1:14 AM
springerm edited the summary of this revision. (Show Details)Feb 2 2023, 7:25 AM

Updated diff to a better strategy for using op dominance. Also a better explanation and more test cases.

Added Ingo just for FYI since we discussed this earlier.

ingomueller-net added inline comments.Feb 6 2023, 1:26 AM
mlir/lib/Dialect/Bufferization/Transforms/OneShotAnalysis.cpp
442–443

I don't fully understand what this does but it looks a bit different from what we had discussed offline: I remember we came to the conclusion that we couldn't use op dominance if the "any common parent region of the read and the write up until the region of the def" was repetitive, right? For this, you'd have to test for any enclosing repetitive region of rRead if it is also an ancestor of uWrite, right? (Maybe your while loop closes too early?)

444

What does this loop do? It doesn't "find" anything, does it, since it doesn't modify anything outside it's scope? The only effect I see is that it asserts...

springerm updated this revision to Diff 495045.Feb 6 2023, 2:23 AM
springerm edited the summary of this revision. (Show Details)

fix missing assignment in loop

springerm marked 2 inline comments as done.Feb 6 2023, 2:26 AM
springerm added inline comments.
mlir/lib/Dialect/Bufferization/Transforms/OneShotAnalysis.cpp
442–443

The loop is looking for a repetitive region that is a child region (maybe not direct child) of rDef. Starting from rRead. The loop was not doing what the comment is saying. Added another test case that runs multiple iterations of this loop.

444

Yes, this was broken. An assignment was missing.

ingomueller-net added inline comments.Feb 6 2023, 2:33 AM
mlir/lib/Dialect/Bufferization/Transforms/OneShotAnalysis.cpp
442–443

Ah, OK, I think I get it. If the "top-most" repetitive region contains both the read and the write (which is what your code is testing), then there is "any common parent region of the read and the right [...] that is repetitive" (which is what I was expecting). Conversely, if the top-most region doesn't contain both, then there is also no region at a deeper nesting level that would contain both.

springerm marked 3 inline comments as done.Feb 6 2023, 3:13 AM
springerm added inline comments.
mlir/lib/Dialect/Bufferization/Transforms/OneShotAnalysis.cpp
442–443

Yes exactly.

This revision was automatically updated to reflect the committed changes.
springerm marked an inline comment as done.