Page MenuHomePhabricator

[mlir][LLVM] Add runtime debug assertions for memref::CollapseShapeOp
AbandonedPublic

Authored by springerm on Nov 21 2022, 9:11 AM.

Details

Summary

This change adds two helper functions: runtimeAbort/runtimeAssert. These can be used to generate runtime assertions.

This change adds runtime asserts to check for invalid memref::CollapseShapeOps. In the static case, these are already detected during op verification. With this change, the program execution crashes explictly at runtime when the assertion is violated. For now, this assertion is activated only on an opt-in basis (to avoid performance regressions).

Diff Detail

Event Timeline

springerm created this revision.Nov 21 2022, 9:11 AM
springerm requested review of this revision.Nov 21 2022, 9:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
springerm retitled this revision from [mlir][LLVM] Add runtime error checking for memref::CollapseShapeOp to [mlir][LLVM] Add runtime debug assertions for memref::CollapseShapeOp.
qcolombet accepted this revision.Nov 21 2022, 3:14 PM

LGTM, couple of nits below.

Thanks for doing this Matthias!

mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1552

Do you expect the following runtimeAssert to be replaced by "nothing"?
Put differently do we need to keep this cast?

1578

Should this be part of this patch?

This revision is now accepted and ready to land.Nov 21 2022, 3:14 PM
springerm marked an inline comment as done.Nov 22 2022, 12:29 AM
springerm added inline comments.
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1552

Just a leftover from debugging

1578

Yes, I just moved it a few lines further down, so that the insertion point is correct when this function is left.

springerm marked an inline comment as done.

address comments

clang-format

nicolasvasilache requested changes to this revision.Nov 22 2022, 11:12 PM
nicolasvasilache added inline comments.
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1636

Big -1 on removing insertion guard here: there exist code paths (i.e. if ref is empty) that change the insertion point and leak across function boundary. This is a huge footgun.

This revision now requires changes to proceed.Nov 22 2022, 11:12 PM
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1636

Note: fix is as simple as adding a guard just before or, better, at the top of the function to be globally safe.

springerm updated this revision to Diff 477415.Nov 23 2022, 1:28 AM

address comments

mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1636

I expect fillInStridesForCollapsedMemDescriptor to actually change the insertion point. This function is creating new ops and when leaving this function it should be set to right after these new ops. (So that runtimeVerifyCollapseShapeContiguity can read from the descriptor that we just filled.)

This is just like reifyResultShapes (and various other helpers that we have); these are also incrementing the insertion point. (It is a bit more complex here because we are splitting blocks, but that is an implementation detail of this function and the caller does not have to worry about that.)

That being said, the rewriter.setInsertionPoint(op); was unnecessarily strict, assuming that this function is only called from the CollapseShapeOp lowering pattern. I fixed that.

springerm planned changes to this revision.Nov 23 2022, 7:44 AM
springerm abandoned this revision.Mon, Jan 30, 2:41 AM