This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][LLVM] Make index type bitwidth configurable.
ClosedPublic

Authored by herhut on Mar 18 2020, 4:27 AM.

Details

Summary

This change adds a new option to the StandardToLLVM lowering to configure
the bitwidth of the index type independently of the target architecture's
pointer size.

Diff Detail

Event Timeline

herhut created this revision.Mar 18 2020, 4:27 AM
ftynse accepted this revision.Mar 18 2020, 6:02 AM

Have you considered using LLVMTypeConverterCustomization for this? If you had and decided otherwise, I'm okay to land this after the comments are addressed.

IMO, it should be now possible to drop the the customization and use the new dispatch features available in TypeConverter.

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

Nit: use the named constant instead

mlir/lib/Conversion/GPUCommon/IndexIntrinsicsOpLowering.h
36–37

Can we just have an accessor in the type converter that returns the bitwidth without having to construct the type?

36–37

Nit: while you are there, let's use camelBack for variable names.

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
136 ↗(On Diff #251034)

Nit: drop trivial braces

139 ↗(On Diff #251034)

leftover debug stmt

3084 ↗(On Diff #251034)

Consider including the value that leads to the machine word size being used, otherwise it may confuse people to have bitwidth=0.

This revision is now accepted and ready to land.Mar 18 2020, 6:02 AM
herhut updated this revision to Diff 251631.Mar 20 2020, 7:20 AM
herhut marked 6 inline comments as done.

Comments...

I did not go with the customization as it was not clear to me how those would compose. One should still be able to select different lowerings for signatures and the index type.

I did not understand your comment about new dispatch features. Can you explain?

mehdi_amini requested changes to this revision.Mar 20 2020, 6:28 PM
mehdi_amini added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
51

Can this be just a field (or a new callback) in this structure instead?

This revision now requires changes to proceed.Mar 20 2020, 6:28 PM
herhut updated this revision to Diff 252795.Mar 26 2020, 4:07 AM

Moving flag into LLVMTypeConverterCustomization.

herhut updated this revision to Diff 252797.Mar 26 2020, 4:12 AM
herhut marked an inline comment as done.

Formatting

@ftynse Could you take another look and approve?

herhut updated this revision to Diff 252820.Mar 26 2020, 6:07 AM

Properly rebase...

ftynse added inline comments.Mar 26 2020, 8:29 AM
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h
199

customizations already has it, let's just have getIndexType() access customizations instead and drop the duplicate field. We store the copy of what is passed in the constructor, so we can freely update it with the value derived from the module if necessary.

herhut updated this revision to Diff 252895.Mar 26 2020, 10:28 AM

Comments.

herhut marked an inline comment as done.Mar 26 2020, 10:29 AM
ftynse accepted this revision.Mar 26 2020, 10:54 AM
herhut updated this revision to Diff 253083.Mar 27 2020, 4:35 AM

Rebase again.

herhut updated this revision to Diff 253086.Mar 27 2020, 4:41 AM

Minor cleanup.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 27 2020, 4:50 AM
This revision was automatically updated to reflect the committed changes.