Page MenuHomePhabricator

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

Authored by frgossen on Wed, Jul 15, 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.Wed, Jul 15, 9:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
frgossen added a reviewer: bondhugula.
rriddle added inline comments.Wed, Jul 15, 10:21 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1429

nit: remove the newline here.

1452

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

1463

Why not use LLVM::CondBranchOp directly here?

1463

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.Wed, Jul 15, 8:36 PM
bondhugula added inline comments.
mlir/test/Conversion/StandardToLLVM/standard-to-llvm.mlir
86

The targets are switched.

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

Address comments

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

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.Thu, Jul 16, 9:58 PM
silvas added inline comments.Thu, Jul 16, 10:00 PM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1443

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

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

Thanks for adding this!

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1443

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.Fri, Jul 17, 12:02 AM
ftynse added inline comments.Fri, Jul 17, 12:29 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1443

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.Mon, Jul 20, 12:01 AM

@bondhugula, can I land this?

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