Page MenuHomePhabricator

[mlir][openacc] Add loop op verifier
ClosedPublic

Authored by clementval on Sep 11 2020, 12:56 PM.

Details

Summary

Add a verifier for the loop op in the OpenACC dialect. Check basic restriction
from 2.9 Loop construct from the OpenACC 3.0 specs.

Diff Detail

Event Timeline

clementval created this revision.Sep 11 2020, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2020, 12:56 PM
clementval requested review of this revision.Sep 11 2020, 12:56 PM
ftynse requested changes to this revision.Sep 14 2020, 7:43 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
259–272

Tablegen generates accessors for all attributes using their names, with proper value casting, loopAuto(), loopIndependent() and loopSeq() in this case. Please use those instead of reimplementing them.

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
665

don't add trailing dots to error messages

668

Nit: terminate sentences with a dot.

675

This can be expressed as a trait in tablegen: either SingleBlockImplicitTerminator if you also want the terminator kind to be verified and automatically added by the parser, or SizedRegion<1> if you only want single-block regions.

mlir/test/Dialect/OpenACC/ops.mlir
196–198

These changes look irrelevant to the commit.

This revision now requires changes to proceed.Sep 14 2020, 7:43 AM
clementval added inline comments.Sep 14 2020, 9:35 AM
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
675

I would like to check that there is something else than the terminator.

ftynse added inline comments.Sep 14 2020, 9:47 AM
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
675

Then use SizedRegion<1> to check for a single block, and then write the proper check for more operations.

clementval marked 5 inline comments as done.

Address review comments

mlir/test/Dialect/OpenACC/ops.mlir
196–198

With the new verifier empty body will trigger an error therefore I added smith in.

rriddle added inline comments.Sep 14 2020, 5:38 PM
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
486

I'm pretty sure you can model this using a DefaultValueAttr in ODS.

671

Missing returns here.

ftynse added inline comments.Sep 15 2020, 1:48 AM
mlir/test/Dialect/OpenACC/ops.mlir
196–198

I'd just add "some.op"() : () -> (), which is shorter and does not imply there is some semantics associated with SCF fors being present under acc.loop.

236

Please don't tests for irrelevant things such as scf.for syntax. It has absolutely no connection to the acc dialect and just makes it more painful for anybody willing to change scf.for itself.

clementval marked 4 inline comments as done.Sep 15 2020, 8:22 AM
clementval added inline comments.
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
486

Sure, I have update the op to use the DefaultValuedAttr and it works fine.

mlir/test/Dialect/OpenACC/ops.mlir
196–198

Sure makes sense. I'll update that.

236

Sure I'll use a dummy here as well.

236

I updated the patch to use a dummy op here as well.

clementval marked 3 inline comments as done.

Rebase + address review comments

ftynse accepted this revision.Sep 15 2020, 8:26 AM
This revision is now accepted and ready to land.Sep 15 2020, 8:26 AM
This revision was landed with ongoing or failed builds.Sep 15 2020, 8:42 AM
This revision was automatically updated to reflect the committed changes.