This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Stop allowing LLVMType Int arguments for GPULaunchFuncOp.
ClosedPublic

Authored by pifon2a on Sep 23 2020, 5:47 AM.

Details

Summary

Conversion to LLVM becomes confusing and incorrect if someone tries to lower
STD -> LLVM and only then GPULaunchFuncOp to LLVM separately. Although it is
technically allowed now, it works incorrectly because of the argument
promotion. The correct way to use this conversion pattern is to add to the
STD->LLVM patterns before running the pass.

Diff Detail

Event Timeline

pifon2a created this revision.Sep 23 2020, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2020, 5:47 AM
pifon2a requested review of this revision.Sep 23 2020, 5:47 AM
pifon2a updated this revision to Diff 293755.Sep 23 2020, 8:33 AM

Removed todo

mehdi_amini added inline comments.Sep 23 2020, 10:04 AM
mlir/test/Conversion/GPUCommon/lower-launch-func-to-gpu-runtime-calls.mlir
26

Seems to indicate that the verifier is fairly loose here with respect to the compatibility of the matching between the call site and callee arguments list?

pifon2a added inline comments.Sep 23 2020, 11:12 AM
mlir/test/Conversion/GPUCommon/lower-launch-func-to-gpu-runtime-calls.mlir
26

that's true. I don't think there is any verification. GPU module is lowered to a binary blob, which will become a global constant. Then GPU LaunchFuncOp will call the code in that blob. Probably, there is a way to verify arguments before we are in LLVM dialect.

mehdi_amini accepted this revision.Sep 23 2020, 12:02 PM
mehdi_amini added inline comments.
mlir/test/Conversion/GPUCommon/lower-launch-func-to-gpu-runtime-calls.mlir
26

Seems like we won't be able to with launch_func right now, it is a bit uncomfortably lose though. The region based one doesn't suffer from this though.

This revision is now accepted and ready to land.Sep 23 2020, 12:02 PM