This is an archive of the discontinued LLVM Phabricator instance.

[mlir:PDLL] Add support for PDL MLIR code generation
ClosedPublic

Authored by rriddle on Feb 14 2022, 2:07 PM.

Details

Summary

This commits starts to plumb PDLL down into MLIR and adds an initial
PDL generator. After this commit, we will have conceptually support
end-to-end execution of PDLL. Followups will add CPP generation to
match the current DRR setup, and begin to add various end-to-end
tests to test PDLL execution.

Diff Detail

Event Timeline

rriddle created this revision.Feb 14 2022, 2:07 PM
rriddle requested review of this revision.Feb 14 2022, 2:07 PM
nicolasvasilache accepted this revision.Feb 15 2022, 1:03 AM

Nice, looking forward to using this!

I'd still be interested in seeing PDLL grammar + doc materialized somewhere, this would have helped review some of the mutual recursions.
Still, the tests are nice and help infer some of this back into working memory.

mlir/lib/Tools/PDLL/CodeGen/MLIRGen.cpp
210

Here and below.
Are we sure we always want to remain in the nested rewrite Op ?

I find this insertion point change across function boundaries to almost always be a source of pain.
How about:

OpBuilder::InsertionGuard g(builder);
checkAndNestUnderRewriteOp(builder, rootExpr, loc);

here and everywhere ?

269

Is there an assertion we can use ?
Seems better to temporarily crash than just "drop it".

275

Thanks!

282

nit: is there a lookupOrNull in this API to avoid 2 calls ?

This revision is now accepted and ready to land.Feb 15 2022, 1:03 AM
jpienaar accepted this revision.Feb 15 2022, 9:16 AM

Nice thanks

mlir/lib/Tools/PDLL/CodeGen/MLIRGen.cpp
47

It would be nice if we had a range Location :)

59

Sort?

73

Comment ?

113

s/mlir/MLIR/

291

Seems this is only place loc is used, could we compute it here only?

373

Given this is an offline generator, I'd rather this produce an error in non-debug builds too as very few people actually build tools with assertions on. (unless no user input can ever get here/would have been checked before here). Same for other asserts.

mehdi_amini added inline comments.Feb 15 2022, 11:02 AM
mlir/lib/Tools/PDLL/CodeGen/MLIRGen.cpp
373

This is supposed to be handled by the frontend though, the verification of the AST and diagnostic hopefully happens there and then the assert is just checking this contract.

rriddle updated this revision to Diff 409070.Feb 15 2022, 3:20 PM
nicolasvasilache accepted this revision.Feb 16 2022, 2:41 AM
nicolasvasilache added inline comments.
mlir/lib/Tools/PDLL/CodeGen/MLIRGen.cpp
373

+1 to defensive programming; if it ever crashes, the frontend should be fixed with a proper error message.

rriddle updated this revision to Diff 409414.Feb 16 2022, 2:42 PM
rriddle marked 11 inline comments as done.
rriddle added inline comments.Feb 16 2022, 2:42 PM
mlir/lib/Tools/PDLL/CodeGen/MLIRGen.cpp
269

I opted for just a FIXME as I have a patch in the stack that handles the PDL plumbing, and dropping it won't cause anything to mis-compile.

282

Thanks, I kept looking for a "find" but didn't notice that it's called "begin".

373

Exactly, this is codifying an expectation that should be verified by the frontend. If we hit a crash here, it's an indication of a bug that needs to be fixed. We shouldn't be surfacing errors from this point, the constructs should have been verified above this layer.

This revision was automatically updated to reflect the committed changes.