Page MenuHomePhabricator

[mlir] introduce data layout entry for index type
ClosedPublic

Authored by ftynse on Mar 19 2021, 3:26 AM.

Details

Summary

Index type is an integer type of target-specific bitwidth present in many MLIR
operations (loops, memory accesses). Converting values of this type to
fixed-size integers has always been problematic. Introduce a data layout entry
to specify the bitwidth of index in a given layout scope, defaulting to 64
bits, which is a commonly used assumption, e.g., in constants.

Port builtin-to-LLVM type conversion to use this data layout entry when
converting index type and untie it from pointer size. This is particularly
relevant for GPU targets. Keep a possibility to forcibly override the index
type in lowerings.

Depends On D98525

Diff Detail

Event Timeline

ftynse created this revision.Mar 19 2021, 3:26 AM
ftynse requested review of this revision.Mar 19 2021, 3:26 AM

Generally looks good. Just one question. Thanks for adding support for this!

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

Now that this is data layout based, should it not query for the local context rather than assuming a global value? E.g., we could lower a module that contains gpu.module all at once and use different index sizes for them. Currently we have to lower in two steps.

ftynse updated this revision to Diff 332274.Mon, Mar 22, 6:58 AM

Rebase

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

This would be a much bigger change potentially affecting core infra: the type converter will have to know in which IR scope it is called, which could further affect things like materialization that currently assumes type conversion is scope-agnostic. It will also need a sort of analysis that maintains the DataLayout cache. I will eventually look into that, but this change enables the data layout-driven flow without breaking the current two-stage approach.

mravishankar resigned from this revision.Mon, Mar 22, 10:45 PM
herhut accepted this revision.Wed, Mar 24, 3:54 AM

Thanks for cleaning this up.

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

Agreed. This makes sense longer term but the current solution does not regress what we currently have. As long as one only applies the conversion to one data layout scope at a time (currently likely a module) this is fine.

This revision is now accepted and ready to land.Wed, Mar 24, 3:54 AM
This revision was landed with ongoing or failed builds.Wed, Mar 24, 7:14 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Wed, Mar 24, 11:47 AM
mlir/docs/DataLayout.md
273

I think this is leaving some confusion in what is the difference between an index and an integer of the provided size?
I.e. if you have a layout that is providing a size for the "index" type, then what is the difference with just using this integer everywhere?
If there are no semantics difference between the index and an integer in such case, it looks like it makes index just a "frontend" type, but I'm not sure why it just shouldn't be immediately "lowered" to an integer when the size is known instead of keeping metadata (otherwise you have two representation for the same thing (a sized integer) which complexify the IR and the compiler in general).