This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Standard] Add default lowering for `assert`
ClosedPublic

Authored by frgossen on Jul 15 2020, 9:48 AM.

Details

Summary

The default lowering of assert calls abort in case the assertion is
violated. The failure message is ignored but should be used by custom lowerings
that can assume more about their environment.

Diff Detail

Event Timeline

frgossen created this revision.Jul 15 2020, 9:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
frgossen added a reviewer: bondhugula.
rriddle added inline comments.Jul 15 2020, 10:21 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1406

nit: remove the newline here.

1429

nit: Can you use Block instead of auto here and below?

1440

Why not use LLVM::CondBranchOp directly here?

1440

This seems incorrect. I would expect the continuation block to be destination if the condition is true. (The true block is first is cond_br)

bondhugula requested changes to this revision.Jul 15 2020, 8:36 PM
bondhugula added inline comments.
mlir/test/Conversion/StandardToLLVM/standard-to-llvm.mlir
98

The targets are switched.

This revision now requires changes to proceed.Jul 15 2020, 8:36 PM
frgossen updated this revision to Diff 278403.Jul 16 2020, 2:27 AM
frgossen marked 5 inline comments as done.

Address comments

ftynse accepted this revision.Jul 16 2020, 9:34 AM
silvas requested changes to this revision.Jul 16 2020, 9:58 PM
silvas added a subscriber: silvas.
silvas added inline comments.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1420

Mutating the module like this is not allowed. This conversion pattern could be applied inside a function pass which will be running in parallel, and this mutation of the module body will trigger a race condition very easily. It's likely that if you craft a test case that simply has two std.assert ops, each in a different function, you will cause a ThreadSanitizer failure.

What is currently being done for creating the malloc/free declarations?

This revision now requires changes to proceed.Jul 16 2020, 9:58 PM
silvas added inline comments.Jul 16 2020, 10:00 PM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1420

Or do we have some sort of guarantee that this is running single-threaded on the whole module?

silvas accepted this revision.Jul 16 2020, 10:10 PM

Thanks for adding this!

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1420

I see that we are already doing this kind of "insert into the mdoule from deep inside a conversion pattern" in a number of places. Sigh. I guess we are assuming that these patterns are being applied serially for now :/

Sorry for the noise.

frgossen marked 3 inline comments as done.Jul 17 2020, 12:02 AM
ftynse added inline comments.Jul 17 2020, 12:29 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1420

Conversion to LLVM is a module pass. It replaces standard functions with llvm functions, so it cannot be a function pass anyway. If it becomes a performance issue, we can potentially split it into two parts: rewrite functions serially, then rewrite function bodies in parallel. This will need some infrastructural support to work around type mismatches (many patterns rely on knowing the original type) and to avoid splitting this into two passes.

frgossen marked an inline comment as done.Jul 20 2020, 12:01 AM

@bondhugula, can I land this?

This revision was not accepted when it landed; it landed in state Needs Revision.Jul 24 2020, 1:31 AM
This revision was automatically updated to reflect the committed changes.