Page MenuHomePhabricator

[mlir] make the bitwidth of device side index computations configurable
ClosedPublic

Authored by gysit on May 20 2020, 2:32 AM.

Details

Summary

The patch makes the index type lowering of the GPU to NVVM/ROCDL conversion configurable. It introduces a pass option that controls the bitwidth used when lowering index computations.

Diff Detail

Event Timeline

gysit created this revision.May 20 2020, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2020, 2:32 AM
gysit added a comment.May 20 2020, 2:38 AM

My assumption is that the LLVMTypeConverterCustomization do not interfere with the address space conversion. Should the address space conversion be an integral part of the LLVMTypeConverterCustomization class?

herhut accepted this revision.May 26 2020, 12:15 AM

My assumption is that the LLVMTypeConverterCustomization do not interfere with the address space conversion. Should the address space conversion be an integral part of the LLVMTypeConverterCustomization class?

This is a bit muddled at the moment but it is ok to assume this for now.

mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp
109

Typo: bidtwidth -> bitwidth

This revision is now accepted and ready to land.May 26 2020, 12:15 AM
gysit updated this revision to Diff 266112.May 26 2020, 12:51 AM

I fixed the typo and added extended the existing tests a little bit to test 32-bit index computations

gysit marked 2 inline comments as done.May 26 2020, 12:53 AM

If we want to strive to little bit bigger refactoring we can postpone landing

mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp
109

fixed

gysit updated this revision to Diff 269436.Jun 9 2020, 1:43 AM
gysit marked an inline comment as done.
gysit edited the summary of this revision. (Show Details)

The update passes the options structure to the type converter and to the conversion pattern base class (replaces the llvm type converter customizations). I also extended the patch to the rocdl backend.

The downside of the patch is that I have to pass the lower to llvm options to all the patters. Theoretically it would also be possible to access the options via the type converter but most of them are not related to the type conversion.

Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2020, 1:43 AM
Herald added a subscriber: msifontes. · View Herald Transcript
gysit updated this revision to Diff 272017.Fri, Jun 19, 4:53 AM

updated to the latest master

This revision was automatically updated to reflect the committed changes.

nn

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

I am puzzled how is this working?

This default value for this parameter is mapped to a reference member, how isn't it gonna lead to "use-after-free"?

gysit marked an inline comment as done.Tue, Jun 23, 2:31 AM
gysit added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

You are right this should not work (the lifetime of the default argument is limited to the body of the constructor -- I believe). I will submit a patch to fix this problem.

ftynse added inline comments.Tue, Jun 23, 3:34 AM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

Hmm, why wouldn't it? The lifetime of the temporary is that of the constructor body. The reference will be used to copy-construct the member struct at the beginning of the constructor implementation, at which point the temporary is guaranteed to be live. Then we will only use the member. It would have been a problem if ConvertToLLVMPattern kept a reference to the temporary.

gysit marked an inline comment as done.Tue, Jun 23, 4:11 AM
gysit added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385
It would have been a problem if ConvertToLLVMPattern kept a reference to the temporary.

The options member is unfortunately a const reference.

ftynse added inline comments.Tue, Jun 23, 4:21 AM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

My bad, I looked elsewhere. The reference capture semantics should be documented somewhere. Or in a more hacky way, this can accept a non-const reference that would effectively disallow passing in temporaries.

gysit marked an inline comment as done.Tue, Jun 23, 5:13 AM
gysit added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

should we just make the options a non-reference member of ConvertToLLVMPattern? At the moment to struct is super small and copying the options should not harm performance.

jeanPerier added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

+ 1, this broke our flang builds with some compilers (they randomly emitted C interface).

mehdi_amini added inline comments.Tue, Jun 23, 9:35 AM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

Duplicating the option in every single pattern instance inheriting from ConvertToLLVMPattern seems a bit suboptimal to me.

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
30

If you reorder the bool before the unsigned, the struct would be smaller I think (on most platform)

gysit marked 3 inline comments as done.Tue, Jun 23, 10:49 AM
gysit added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

+ 1, this broke our flang builds with some compilers (they randomly emitted C interface).

Sorry for breaking your build. I reverted the commit which hopefully solves your problem.

385

Duplicating the option in every single pattern instance inheriting from ConvertToLLVMPattern seems a bit suboptimal to me.

Using a reference or a pointer to the options structure are possible alternatives. Both of them have memory lifetime issues if the referenced memory is freed to early. An alternative could be to pass in a callback that returns an options structure (similar to the one used for the type converter before). This solution has no lifetime issues and the memory footprint should be minimal (a function pointer).

gysit marked an inline comment as done.Tue, Jun 23, 10:59 AM
mehdi_amini added inline comments.Tue, Jun 23, 12:56 PM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

I'm missing something: how is the lifetime of the callback managed? In some way if you can pass a pointer to a callback that returns the data, you can also provide a pointer to the data.

gysit added inline comments.Tue, Jun 23, 2:28 PM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

The idea was to use a local static variable

LowerToLLVMOptions myConfigOptions() {
  static const LowerToLLVMOptions myOptions = { /* ... */ };
  return myOptions;
}

However, this solution only works for the options that can be statically initialized (such as the default options) and pointers/references are still needed for the runtime pass parameters. So having pointers / references everywhere may be the better solution.

mehdi_amini added inline comments.Tue, Jun 23, 4:27 PM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

In particular, if you can have a static you can also pass it by reference :)

jeanPerier added inline comments.Tue, Jun 23, 11:31 PM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
385

Thanks, this part of flang code is still in a fork so it was hard for you to know. What went wrong in our builds was the default arguments. They ended-up being temps with the lifetime of the ctor call. Creating the default LowerToLLVMOptions on our side and passing it to populateStdToLLVMConversionPatterns was working OK.