Details

Reviewers

mehdi_amini
ftynse
silvas
tpopp
nicolasvasilache
dcaballe
rriddle

Commits

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

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

Repository: rG LLVM Github Monorepo

Event Timeline

springerm created this revision.Nov 24 2022, 3:09 AM

Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2022, 3:09 AM

Herald added subscribers: Moerafaat, zero9178, bzcheeseman and 21 others. · View Herald Transcript

springerm requested review of this revision.Nov 24 2022, 3:09 AM

Herald added a reviewer: nicolasvasilache. · View Herald TranscriptNov 24 2022, 3:09 AM

Herald added a reviewer: dcaballe. · View Herald Transcript

Herald added a project: Restricted Project. · View Herald Transcript

Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript

springerm added inline comments.Nov 24 2022, 3:12 AM

mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
97–100	Note: This (copying StringRef into `uint8_t` vector) is copied from LLVM: `llvm/lib/IR/Constants.cpp` (`ConstantDataArray::getString`).

Harbormaster completed remote builds in B199370: Diff 477718.Nov 24 2022, 3:54 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
81	If we do this, we need to limit the pass to only running on Modules or risk race conditions.
90	I would rename this to something like `createPrintStr`. Otherwise, I thought this was immediately printing something rather than generating a print statement.
132	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 edited the summary of this revision. (Show Details)Nov 24 2022, 7:09 AM

springerm added a parent revision: D138576: [mlir] Add RuntimeVerifiableOpInterface and transform.

address comments

mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
81	`ConvertControlFlowToLLVM` is already a Module pass.

springerm added a child revision: D138671: [mlir][memref] Add runtime verification for memref::CastOp.Nov 24 2022, 7:12 AM

rriddle added inline comments.Nov 24 2022, 7:15 AM

mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
81	Only classes should be in anonymous namespaces, pull these out and mark static.

Harbormaster completed remote builds in B199422: Diff 477782.Nov 24 2022, 7:18 AM

springerm added a reviewer: rriddle.Nov 24 2022, 7:18 AM

fix cmake

springerm marked an inline comment as done.Nov 24 2022, 7:24 AM

Harbormaster completed remote builds in B199426: Diff 477788.Nov 24 2022, 7:41 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
132	I agree with this, proliferations of "options" is something I'm concerned about in general and better be avoided when we can.

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
132	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.