This is an archive of the discontinued LLVM Phabricator instance.

[mlir][openacc] Change operand type from index to AnyInteger in parallel op
ClosedPublic

Authored by clementval on Sep 15 2020, 11:41 AM.

Details

Summary

This patch change the type of operands async, wait, numGangs, numWorkers and vectorLength from index
to AnyInteger to fit with acc.loop and the OpenACC specification.

Diff Detail

Event Timeline

clementval created this revision.Sep 15 2020, 11:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 11:41 AM
clementval requested review of this revision.Sep 15 2020, 11:41 AM
ftynse requested changes to this revision.Sep 16 2020, 12:51 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
96–100

Make sure index is also supported. It was intended as a platform-specific opaque size type. While it's reasonable to adhere to the spec and allow any integer, most lowerings coming from higher levels and many standard ops (e.g. taking the size of some container) will use index.

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
162–165

This will now parse every type as i64, regardless of what type is actually used. Roundtripping will thus erase type information, which is incorrect. If you need to support different types, you need to specify which type is used somehow.

This revision now requires changes to proceed.Sep 16 2020, 12:51 AM
clementval added inline comments.Sep 16 2020, 7:01 AM
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
162–165

Right, I was suspecting smith was not quite right here. I guess I have to add the type and parse it as well like I did for the variadic operands in this op. I'll update the patch today.

clementval marked 2 inline comments as done.Sep 16 2020, 6:35 PM
clementval added inline comments.
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
96–100

I update this to use AnyTypeOf<[AnyInteger, Index]>

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
162–165

This has been change with parsing the actual type as well.

clementval marked an inline comment as done.

Address review comments

Herald added a project: Restricted Project. · View Herald Transcript
clementval added inline comments.Sep 16 2020, 6:46 PM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
96–100

If that's fine I'll make a follow up patch to use the same type in acc.loop

Remove unrelated commits

ftynse accepted this revision.Sep 17 2020, 4:40 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
96–100

Yes, thanks!

You may want to define def IntOrIndex : AnyTypeOf<[...]>; and use it instead of repeating every time.

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

We have OptionalParseResult for this.

This revision is now accepted and ready to land.Sep 17 2020, 4:40 AM
clementval marked 2 inline comments as done.

Address review comments

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
96–100

Sure makes sense. Patch is updated.

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

Thanks for the pointer. I was not aware of it. I updated this function and I'll update the reset in a follow up patch.

This revision was landed with ongoing or failed builds.Sep 17 2020, 8:34 AM
This revision was automatically updated to reflect the committed changes.