This is an archive of the discontinued LLVM Phabricator instance.

buffer-deallocation: consider aliases introduced by arith.select.
ClosedPublic

Authored by jreiffers on Aug 23 2022, 4:12 AM.

Details

Summary

Currently, buffer deallocation considers arith.select to be
non-aliasing, which results in deallocs being inserted incorrectly. Since
arith.select doesn't implement any useful interfaces, this change just handles
it explicitly. Eventually this should probably be fixed properly, if this pass
is going to be used long term.

Diff Detail

Event Timeline

jreiffers created this revision.Aug 23 2022, 4:12 AM
jreiffers requested review of this revision.Aug 23 2022, 4:12 AM
jreiffers updated this revision to Diff 454796.Aug 23 2022, 5:02 AM

Remove accidental --dump-input.

springerm accepted this revision.Aug 23 2022, 5:15 AM
springerm added inline comments.
mlir/lib/Analysis/BufferViewFlowAnalysis.cpp
124

Maybe put a TODO that we need an interface here to avoid listing ops. (And adding dialect dependencies to Analysis.)

utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
5882

This is a bit unfortunate. But I don't see a good alternative without adding a new interface.

CMake file also needs updating.

This revision is now accepted and ready to land.Aug 23 2022, 5:15 AM
jreiffers updated this revision to Diff 454799.Aug 23 2022, 5:22 AM

Fix CMakeLists, add a TODO.

jreiffers marked 2 inline comments as done.Aug 23 2022, 5:22 AM

Thanks!

mlir/lib/Analysis/BufferViewFlowAnalysis.cpp
124

Done.

utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
5882

Done.

This revision was landed with ongoing or failed builds.Aug 23 2022, 5:37 AM
This revision was automatically updated to reflect the committed changes.
jreiffers marked 2 inline comments as done.
herhut added a subscriber: herhut.Aug 23 2022, 8:24 AM

I find it very strange that arith.select supports memref in the first place. Are there other operations in the arith dialect that do this?

I find it very strange that arith.select supports memref in the first place. Are there other operations in the arith dialect that do this?

I think this is because arith.select used to be std.select and there was no better place to put it. (Maybe the scf dialect would be an alternative.)

rriddle added inline comments.Aug 29 2022, 11:46 AM
mlir/lib/Analysis/BufferViewFlowAnalysis.cpp
124

What is the timeline for fixing this? We should not be adding dialect dependencies to Analysis. Is this going to be fixed soon? Otherwise, please revert.

jreiffers added inline comments.Aug 29 2022, 10:42 PM
mlir/lib/Analysis/BufferViewFlowAnalysis.cpp
124

Ah, thank you for pinging this. I'd be happy to fix this soon, but there should probably be a little bit of discussion on how to do it first. I don't see any existing interface that quite fits here. It might be possible to generalize BranchOpInterface, but to me it doesn't seem like it's going to work well. We could add a new interface to ControlFlowInterfaces (strawman proposal: ValueBranchOpInterface), but that doesn't seem like a perfect match either. Or we could create a new interface elsewhere (but as far as I can tell, it would be a singleton for now, whatever it would end up being. Maybe something specifically around memory aliasing). Input or better ideas welcome.

springerm added inline comments.Aug 30 2022, 12:10 AM
mlir/lib/Analysis/BufferViewFlowAnalysis.cpp
124

I'd say let's move the entire BufferViewFlowAnalysis to the BufferizationTransforms. We only use it for bufferization, so it would make sense to have it there. And we can have a dependency on the Arith dialect there.

Step 2, we can add a new interface that is somewhat similar to BufferizableOpInterface::getAliasingOpResult, but operating on buffers. E.g., we could call it BufferAliasingOpInterface.

Step 3, no longer use RegionBranchOpInterface etc. in BufferViewFlowAnalysis (which is used incorrectly at the moment), but only use BufferAliasingOpInterface. To that end, we have to implement this interface on a few other ops: scf.for, scf.while, scf.execute_region, memref.subview, maybe a few more. The list of ops that must implement this interface is actually quite small. Also, the default case in the analysis (if an op does not implement the interface) should be "assume that operand and result" are aliasing. Unknown ops are currently treated as "no aliasing", which is incorrect.

jreiffers added inline comments.Aug 30 2022, 4:16 AM
mlir/lib/Analysis/BufferViewFlowAnalysis.cpp
124

OK, I like that solution. https://reviews.llvm.org/D132928 should do step 1 and resolve the immediate issue here. Thanks!