[mlir] Do not generate sizeof expressions using GEP

[nikic]

Lowering MemRef to LLVM dialect currently generates ptrtoint (getelementptr null, 1) style sizeof expressions using this helper:

llvm-project/mlir/lib/Conversion/LLVMCommon/Pattern.cpp

Lines 171 to 184 in cfb4dce

	Value ConvertToLLVMPattern::getSizeInBytes(
	Location loc, Type type, ConversionPatternRewriter &rewriter) const {
	// Compute the size of an individual element. This emits the MLIR equivalent
	// of the following sizeof(...) implementation in LLVM IR:
	// %0 = getelementptr %elementType* null, %indexType 1
	// %1 = ptrtoint %elementType* %0 to %indexType
	// which is a common pattern of getting the size of a type in bytes.
	Type llvmType = typeConverter->convertType(type);
	auto convertedPtrType = LLVM::LLVMPointerType::get(rewriter.getContext());
	auto nullPtr = rewriter.create<LLVM::ZeroOp>(loc, convertedPtrType);
	auto gep = rewriter.create<LLVM::GEPOp>(loc, convertedPtrType, llvmType,
	nullPtr, ArrayRef<LLVM::GEPArg>{1});
	return rewriter.create<LLVM::PtrToIntOp>(loc, getIndexType(), gep);
	}

These need to be replaced with an emission of a constant based on the DataLayout.

The ability to generate the expressions in this form will go away entirely with https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699 and causes problems for https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179 as well.

[llvmbot]

@llvm/issue-subscribers-mlir-llvm

Author: Nikita Popov (nikic)

Lowering MemRef to LLVM dialect currently generates `ptrtoint (getelementptr null, 1)` style sizeof expressions using this helper: https://github.com/llvm/llvm-project/blob/cfb4dce90ce66acbd094e443f422c4e5c413a9e8/mlir/lib/Conversion/LLVMCommon/Pattern.cpp#L171-L184

These need to be replaced with an emission of a constant based on the DataLayout.

The ability to generate the expressions in this form will go away entirely with https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699 and causes problems for https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179 as well.

[krzysz00]

A potential problem is that aot of uses of ConvertToLLVM won't have the data layout available at the time those conversion patterns are running

That's probably something that needs to be fixed long-term

Perhaps a llvm.mlir.sizeof that gets translated at module translation time would work as a quick fix

[krzysz00]

@rengolin and @fabianmcg and maybe some other people since this is related to targets/codegen/... and is a useful concrete thing for a new system to do

[fabianmcg]

@nikic do you have a timeline as to when are they are going away and when ptradd is landing? I'm adding a solution to that issue on MLIR, however, I don't have an ETA as to when it will land -It's in review, but it's a +6k loc code change.

So, depending on your ETA a quicker solution might be required.

If I recall correctly it's always possible to query the data layout, but it might not correspond to actual expected size if the target has not been determined.

[rengolin]

That's an interesting take. The main issue is that, unlike LLVM IR, MLIR does not mandate data layout to exist in IR. But if it exists, I think it would be a nice way to fixing the problem.

Other issues:

• DLTI allows listing more than one target (in the TI section) but so far we only have examples of one data layout (in the DL section). We may have to move/add data layout to the target description or at least require some hierarchy that can be queried in a generic way.
• Data layout only talks about scalars (including pointers), not aggregates and their ABI requirements. C++ has a set of structure layout ABIs (Itanium, Arm) that are not trivial to encode and would need to be fully encoded in the GEP lowering.

Having said that, I believe Clang will have to deal with that problem in CIL, so perhaps there's an interest in involving them in this discussion?

@AaronBallman

[fabianmcg]

• DLTI allows listing more than one target (in the TI section) but so far we only have examples of one data layout (in the DL section). We may have to move/add data layout to the target description or at least require some hierarchy that can be queried in a generic way.

If we are at a stage with LLVM in the IR, then I think we can assume/force only having a single layout and a single target.
For example, it would be odd to have rocdl and nvvm ops at that stage.

• Data layout only talks about scalars (including pointers), not aggregates and their ABI requirements. C++ has a set of structure layout ABIs (Itanium, Arm) that are not trivial to encode and would need to be fully encoded in the GEP lowering.

If llvm is thinking to derive those constants from the DL, then we should be able to do it too, we only need to require a compilation target before lowering to LLVM.

I believe Clang will have to deal with that problem in CIL, so perhaps there's an interest in involving them in this discussion?

That code would likely live in clang. However, if we shoot for the moon we could finally be able to have C++ calling conventions on MLIR (and for that matter for all programming languages using MLIR). But in the best-case scenario, that's going to take years.

[nikic]

A potential problem is that aot of uses of ConvertToLLVM won't have the data layout available at the time those conversion patterns are running

That's probably something that needs to be fixed long-term

I'm not at all familiar with MLIR -- my expectation would have been that as LLVM IR always requires a DataLayout (though it may just be the implicit default data layout that is used for tests) that the MLIR LLVM IR dialect would mirror that. Is that not architecturally possible with MLIR?

@nikic do you have a timeline as to when are they are going away and when ptradd is landing? I'm adding a solution to that issue on MLIR, however, I don't have an ETA as to when it will land -It's in review, but it's a +6k loc code change.

This is not urgent. This is more of a "this will break in a few LLVM releases" thing than "this will break in a few days".

[AaronBallman]

That's an interesting take. The main issue is that, unlike LLVM IR, MLIR does not mandate data layout to exist in IR. But if it exists, I think it would be a nice way to fixing the problem.

Other issues:

* DLTI allows listing more than one target (in the TI section) but so far we only have examples of one data layout (in the DL section). We may have to move/add data layout to the target description or at least require some hierarchy that can be queried in a generic way.

* Data layout only talks about scalars (including pointers), not aggregates and their ABI requirements. C++ has a set of structure layout ABIs (Itanium, Arm) that are not trivial to encode and would need to be fully encoded in the GEP lowering.

Having said that, I believe Clang will have to deal with that problem in CIL, so perhaps there's an interest in involving them in this discussion?

@AaronBallman

CC @bcardosolopes @lanza for CIR awareness, and @rjmccall @efriedma-quic for Clang codegen awareness.

[nikic]

Data layout only talks about scalars (including pointers), not aggregates and their ABI requirements. C++ has a set of structure layout ABIs (Itanium, Arm) that are not trivial to encode and would need to be fully encoded in the GEP lowering.

The way this works currently is that there is a complex contract between the frontend and LLVM, where the frontend performs certain contortions to produce types that LLVM will then hopefully interpret in the way the frontend intended and produce correct sizes and offsets from it. The model we're slowly moving towards is that all the layout logic is handled exclusively by the frontend and LLVM is only told the computed quantities, so that both sides can not go out of sync. (Where "frontend" in this context may be MLIR/CIL rather than Clang proper.)

[fabianmcg]

Is that not architecturally possible with MLIR?

It's possible to require it. It's just that it's not a requirement and in consequence many users set that information until translation to llvm IR. But with this proposed change we would need to always require it (we already should be doing it).