This is an archive of the discontinued LLVM Phabricator instance.

[mlir][cf] Print message in cf.assert to LLVM lowering
ClosedPublic

Authored by springerm on Nov 24 2022, 3:09 AM.

Details

Summary

The assert message was previously ignored. This change adds a new helper function printStr to the runtime library and calls it in case of a failed assertion.

Depends On D138576

Diff Detail

Event Timeline

springerm created this revision.Nov 24 2022, 3:09 AM
springerm requested review of this revision.Nov 24 2022, 3:09 AM
springerm added inline comments.Nov 24 2022, 3:12 AM
mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
98–101

Note: This (copying StringRef into uint8_t vector) is copied from LLVM: llvm/lib/IR/Constants.cpp (ConstantDataArray::getString).

tpopp added a comment.Nov 24 2022, 6:42 AM

I'm leaving a few high level comments first. I think this abortOnFailedAssert=false case should be handled by a separate test pass instead.

mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
82

If we do this, we need to limit the pass to only running on Modules or risk race conditions.

91

I would rename this to something like createPrintStr. Otherwise, I thought this was immediately printing something rather than generating a print statement.

133

This is convenient, but I don't think it's a great idea to provide this flag that changes the meanings of assertions in the primary populateControlFlowToLLVM. I think there should be a separate test pass that creates prints instead if you want this.

mlir/lib/ExecutionEngine/CRunnerUtils.cpp
50 ↗(On Diff #477718)

I'm just explicitly mentioning that I don't know the consequences of creating this functionality when reviewing the rest of this code.

springerm updated this revision to Diff 477782.Nov 24 2022, 7:11 AM
springerm marked 2 inline comments as done.
springerm edited the summary of this revision. (Show Details)

address comments

mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
82

ConvertControlFlowToLLVM is already a Module pass.

rriddle added inline comments.Nov 24 2022, 7:15 AM
mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
82

Only classes should be in anonymous namespaces, pull these out and mark static.

springerm marked an inline comment as done.Nov 24 2022, 7:24 AM
mehdi_amini added inline comments.Nov 25 2022, 11:12 AM
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
476 ↗(On Diff #477788)

Can't you just call libc in the lowering directly?

476 ↗(On Diff #477788)

That is: I'm concerned about any general lowering to LLVM that would depend on the runner support library right now.

mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
133

I agree with this, proliferations of "options" is something I'm concerned about in general and better be avoided when we can.

springerm updated this revision to Diff 478173.Nov 28 2022, 2:54 AM
springerm marked 2 inline comments as done.

address comments

springerm marked 2 inline comments as done.Nov 28 2022, 2:55 AM
springerm added inline comments.
mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
133

Kept the flag here but it is no longer exposed to populateControlFlowToLLVM. There's a special populate just for this pattern and a new test pass.

springerm updated this revision to Diff 480027.Dec 5 2022, 2:28 AM

minor update

mehdi_amini added inline comments.Dec 14 2022, 7:05 AM
mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h
34

set to false

mlir/test/Integration/Dialect/ControlFlow/assert.mlir
5

Do we need any utils here?

springerm updated this revision to Diff 483093.Dec 15 2022, 1:32 AM
springerm marked an inline comment as done.

address comments

mehdi_amini accepted this revision.Dec 15 2022, 6:27 AM

The commit message probably needs an update.

This revision is now accepted and ready to land.Dec 15 2022, 6:27 AM
This revision was landed with ongoing or failed builds.Dec 15 2022, 8:45 AM
This revision was automatically updated to reflect the committed changes.