Page MenuHomePhabricator

[mlir][openacc] Add missing attributes and operands for acc.loop
ClosedPublic

Authored by clementval on Aug 27 2020, 6:25 PM.

Details

Summary

This patch add the missing operands to the acc.loop operation. Only the device_type
information is not part of the operation for now.

Diff Detail

Event Timeline

clementval created this revision.Aug 27 2020, 6:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2020, 6:25 PM
clementval requested review of this revision.Aug 27 2020, 6:25 PM
clementval edited the summary of this revision. (Show Details)

Change I1Attr to UnitAttr

rriddle added inline comments.Aug 28 2020, 1:00 PM
mlir/include/mlir/Dialect/OpenACC/OpenACC.h
38

This seems unrelated?

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
231

nit: UnitAttr is already an Optional construct, so this is redundant.

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
483–484

nit: Remove this extra newline.

612

nit: Nest this within the existing if for gangNum

621–635

nit: You don't need the == ... here.

630

Same here.

LGTM. Please wait for @rriddle's approval.

mlir/test/Dialect/OpenACC/ops.mlir
79

Is it required to have the name attributes?

This revision is now accepted and ready to land.Aug 28 2020, 2:29 PM
clementval marked 7 inline comments as done.

Address review comments

@rriddle and @kiranchandramohan Thanks for the reviews. I just updated the patche to address the comments.

mlir/include/mlir/Dialect/OpenACC/OpenACC.h
38

Seq is not really part of exec mapping but in group of attributes that includes auto, independent and seq and as they are mutually exclusive it doesn't really belong here anymore.

mlir/test/Dialect/OpenACC/ops.mlir
79

Those are UnitAttr so if they are present it means that are "active".

@rriddle Are you fine with the changes?

mlir/test/Dialect/OpenACC/ops.mlir
79

Sorry, I guess you meant the attributes keyword before the attributes-dict? Yes it is required. I used ParseResult parseOptionalAttrDictWithKeyword(NamedAttrList &result) in the parser for those ops so you need the keyword.

rriddle accepted this revision.Aug 31 2020, 1:33 PM
rriddle added inline comments.
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
518

nit: Drop trivial braces here.

Address review comments

This revision was landed with ongoing or failed builds.Aug 31 2020, 4:50 PM
This revision was automatically updated to reflect the committed changes.