Page MenuHomePhabricator

[mlir][GPU] make the index bitwidth configurable during the GPU to LLVM conversion
Needs ReviewPublic

Authored by gysit on Jan 21 2021, 12:06 PM.

Details

Reviewers
herhut
csigg
Summary

The patch exposes the LLVM lowering options of the GPU to LLVM conversion pass. Additionally, it introduces the logic needed to cast index type parameters passed to the GPU runtime wrapper if the bitwidth of the index type is set to less than the 64-bits used by the GPU runtime wrapper.

For the review: This is a rather intrusive change and it may interfere with the entire data layout refactoring. If you prefer I can maintain this as a local change.

Diff Detail

Event Timeline

gysit created this revision.Jan 21 2021, 12:06 PM
gysit requested review of this revision.Jan 21 2021, 12:06 PM

Hi Tobias!

I'm curious, wouldn't it be easier to compile the runtime wrappers for 32bit instead of casting the arguments?

mehdi_amini added inline comments.Mon, Feb 22, 2:43 PM
mlir/include/mlir/Conversion/Passes.td
127

You're adding many options that I don't find tests for at the moment?

Hi Tobias!

I'm curious, wouldn't it be easier to compile the runtime wrappers for 32bit instead of casting the arguments?

Hi Christian,

My idea was to keep things isolated in the pass and not touch the runtime wrapper. I didn't think about 32-bit compilation. It seems like a clean solution assuming all the rocm / cuda libraries work together with a 32-bit binary? An alternative could be to use preprocessor directives in the wrapper. I am not a big fan of that solution but it may be cleaner / require less code (there are multiple wrappers though).

I suggest we abandon this revision for the time being (we can apply the patch locally in our project). Without the casting it basically boils down to adding the llvm lowering options which should probably be replaced by data layout attributes anyways?

mlir/include/mlir/Conversion/Passes.td
127

That is a good point! I test only the option I actually use and added the other ones for completeness. I should have added and tested only the ones I use instead.

AFAIK the llvm lowering options are replaced by data layout attributes anyways? I think that is a much nicer solution not only from a testing perspective. I thus suggest to abandon this revision (see my answer to @csigg 's comment).