[flang] In AllocMemOp lowering, convert types for calling malloc on 32-bit #129308
Conversation
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-fir-hlfir Author: R (ArcaneNibble) ChangesI came across @georgestagg's work on getting Flang to target WebAssembly and attempted to clean up the hacks so that it can actually be upstreamed. This patch fixes dynamic allocations, and I am submitting it first as it is less invasive than the work needed to fix RTBuilder. This change first ensures that the MLIR->LLVM lowering pass actually has access to the real target DataLayout, as it was previously making do with a default one. The AllocMemOp lowering then inserts a type conversion if the target has 32-bit pointers. This assumes that I haven't checked whether anything else in CodeGen needs changing for cross-compiling (especially since I'm not actually familiar with Fortran), but this change should make any necessary future fixes easier. Full diff: https://github.com/llvm/llvm-project/pull/129308.diff 2 Files Affected:
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index aaefe675730e1..b6ce5f328b61b 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -982,7 +982,8 @@ struct EmboxCharOpConversion : public fir::FIROpConversion<fir::EmboxCharOp> {
template <typename ModuleOp>
static mlir::SymbolRefAttr
getMallocInModule(ModuleOp mod, fir::AllocMemOp op,
- mlir::ConversionPatternRewriter &rewriter) {
+ mlir::ConversionPatternRewriter &rewriter,
+ bool addr32) {
static constexpr char mallocName[] = "malloc";
if (auto mallocFunc =
mod.template lookupSymbol<mlir::LLVM::LLVMFuncOp>(mallocName))
@@ -992,7 +993,7 @@ getMallocInModule(ModuleOp mod, fir::AllocMemOp op,
return mlir::SymbolRefAttr::get(userMalloc);
mlir::OpBuilder moduleBuilder(mod.getBodyRegion());
- auto indexType = mlir::IntegerType::get(op.getContext(), 64);
+ auto indexType = mlir::IntegerType::get(op.getContext(), addr32 ? 32 : 64);
auto mallocDecl = moduleBuilder.create<mlir::LLVM::LLVMFuncOp>(
op.getLoc(), mallocName,
mlir::LLVM::LLVMFunctionType::get(getLlvmPtrType(op.getContext()),
@@ -1003,11 +1004,12 @@ getMallocInModule(ModuleOp mod, fir::AllocMemOp op,
/// Return the LLVMFuncOp corresponding to the standard malloc call.
static mlir::SymbolRefAttr
-getMalloc(fir::AllocMemOp op, mlir::ConversionPatternRewriter &rewriter) {
+getMalloc(fir::AllocMemOp op, mlir::ConversionPatternRewriter &rewriter,
+ bool addr32) {
if (auto mod = op->getParentOfType<mlir::gpu::GPUModuleOp>())
- return getMallocInModule(mod, op, rewriter);
+ return getMallocInModule(mod, op, rewriter, addr32);
auto mod = op->getParentOfType<mlir::ModuleOp>();
- return getMallocInModule(mod, op, rewriter);
+ return getMallocInModule(mod, op, rewriter, addr32);
}
/// Helper function for generating the LLVM IR that computes the distance
@@ -1057,6 +1059,7 @@ struct AllocMemOpConversion : public fir::FIROpConversion<fir::AllocMemOp> {
mlir::Type heapTy = heap.getType();
mlir::Location loc = heap.getLoc();
auto ity = lowerTy().indexType();
+ auto addr32 = lowerTy().getPointerBitwidth(0) == 32;
mlir::Type dataTy = fir::unwrapRefType(heapTy);
mlir::Type llvmObjectTy = convertObjectType(dataTy);
if (fir::isRecordWithTypeParameters(fir::unwrapSequenceType(dataTy)))
@@ -1067,7 +1070,11 @@ struct AllocMemOpConversion : public fir::FIROpConversion<fir::AllocMemOp> {
for (mlir::Value opnd : adaptor.getOperands())
size = rewriter.create<mlir::LLVM::MulOp>(
loc, ity, size, integerCast(loc, rewriter, ity, opnd));
- heap->setAttr("callee", getMalloc(heap, rewriter));
+ if (addr32) {
+ auto i32ty = mlir::IntegerType::get(rewriter.getContext(), 32);
+ size = integerCast(loc, rewriter, i32ty, size);
+ }
+ heap->setAttr("callee", getMalloc(heap, rewriter, addr32));
rewriter.replaceOpWithNewOp<mlir::LLVM::CallOp>(
heap, ::getLlvmPtrType(heap.getContext()), size,
addLLVMOpBundleAttrs(rewriter, heap->getAttrs(), 1));
diff --git a/flang/lib/Optimizer/CodeGen/TypeConverter.cpp b/flang/lib/Optimizer/CodeGen/TypeConverter.cpp
index 89f498433806e..c22a64bd7be7d 100644
--- a/flang/lib/Optimizer/CodeGen/TypeConverter.cpp
+++ b/flang/lib/Optimizer/CodeGen/TypeConverter.cpp
@@ -28,10 +28,22 @@
namespace fir {
+static mlir::LowerToLLVMOptions MakeLowerOptions(mlir::ModuleOp module) {
+ llvm::StringRef dataLayoutString;
+ auto dataLayoutAttr = module->template getAttrOfType<mlir::StringAttr>(
+ mlir::LLVM::LLVMDialect::getDataLayoutAttrName());
+ if (dataLayoutAttr)
+ dataLayoutString = dataLayoutAttr.getValue();
+
+ auto options = mlir::LowerToLLVMOptions(module.getContext());
+ options.dataLayout = llvm::DataLayout(dataLayoutString);
+ return options;
+}
+
LLVMTypeConverter::LLVMTypeConverter(mlir::ModuleOp module, bool applyTBAA,
bool forceUnifiedTBAATree,
const mlir::DataLayout &dl)
- : mlir::LLVMTypeConverter(module.getContext()),
+ : mlir::LLVMTypeConverter(module.getContext(), MakeLowerOptions(module)),
kindMapping(getKindMapping(module)),
specifics(CodeGenSpecifics::get(
module.getContext(), getTargetTriple(module), getKindMapping(module),
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ArcaneNibble
force-pushed
the
wasm-flang-fixes-maybe-pr
branch
from
ef5f230 to
8654d4b
Compare
| @@ -992,7 +992,7 @@ getMallocInModule(ModuleOp mod, fir::AllocMemOp op, | |||
| return mlir::SymbolRefAttr::get(userMalloc); | |||
|
|
|||
| mlir::OpBuilder moduleBuilder(mod.getBodyRegion()); | |||
| auto indexType = mlir::IntegerType::get(op.getContext(), 64); | |||
| auto indexType = mlir::IntegerType::get(op.getContext(), addr32 ? 32 : 64); | |||
There was a problem hiding this comment.
LLVMTypeConverter has a getIndexTypeBitwidth() function. Probably better that if everywhere.
There was a problem hiding this comment.
I just refactored it to use that function. However, (without overriding it) this function doesn't return 32 as required, because the default size in mlir/lib/Interfaces/DataLayoutInterfaces.cpp is also hardcoded as 64.
I was considering fixing DataLayoutImporter::translateDataLayout so that it uses "the size of pointers in the default address space" as the bit width of the index type, but I didn't do that for now because I am not sure if this is correct for all targets and users of MLIR. Could you or someone else chime in?
ArcaneNibble
force-pushed
the
wasm-flang-fixes-maybe-pr
branch
3 times, most recently
from
9fa8953 to
9f4854f
Compare
|
ping? |
There was a problem hiding this comment.
The changes look good to me, but please wait for @clementval's approval.
|
Add a test please. |
ArcaneNibble
force-pushed
the
wasm-flang-fixes-maybe-pr
branch
from
9f4854f to
1303f73
Compare
|
I just added a test. As a stopgap, I've disabled all the existing tests which invoke |
|
Precommit CI is still failing. Can you have a look? I think because you added some line, some runtime function calls that take the line number for error reporting have their value changed. |
ArcaneNibble
force-pushed
the
wasm-flang-fixes-maybe-pr
branch
from
1303f73 to
3c599c1
Compare
|
Hopefully it's fixed now, I didn't expect that something like that could happen. |
Yeah it totally depends how the checks in the tests are written. In this case they were hardcoded so any new line would add one to it. |
|
Tests are passing now |
Thanks! I didn't really follow, but why the 3-4 tests need the require clause? Are they the only test with -emit-llvm? |
|
They're the only tests I found that generate a call to the external Previously, as far as I can tell, the 32-bit code path wasn't being tested at all (because there's minimal-to-no cross-compiling tests as flang doesn't currently properly support it, and all the builders are 64-bit). WebAssembly is unique in being a "new" target which is still 32-bit (32-bit address space, but it has native support for calculations with 64-bit integers). |
Ok. Just to make sure. Flang is not supporting 32-bit target and might never fully support them. You might want to add a disclaimer in your commit message about it. Do you have plan to work further on than? |
|
I do understand that it's not an upstream priority to support these targets. I personally see potential in WebAssembly as a platform in general, and so I'm trying to upstream fixes if possible, even if it doesn't result in it being "officially-supported". re edit: I have immediate plans to try to upstream enough fixes so that the examples in the linked blog post work without horrible hacks (ad-hoc changing some values to |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/21870 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/18167 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/130/builds/10936 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/50/builds/11195 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/172/builds/10593 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/80/builds/11062 Here is the relevant piece of the build log for the reference |
|
ah crud, do those builders not enable all targets? :/ |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/29/builds/11454 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/20/builds/9782 Here is the relevant piece of the build log for the reference |
|
Sorry about that, I'll have to look into how to make this test only run on the x86 buildbot |
…lloc on 32-bit (llvm#129308)" The alloc-32.fir test is now marked as requiring the X86 target.
…lloc on 32-bit (llvm#129308)" Changes: * The alloc-32.fir test is now marked as requiring the X86 target. * Drive-by fixes uncovered by fixing tests involving malloc
…lloc on 32-bit (llvm#129308)" Changes: * The alloc-32.fir test is now marked as requiring the X86 target. * Drive-by fixes uncovered when fixing tests involving malloc
…alling malloc on 32-bit (#130386) Previous PR: llvm/llvm-project#129308 Changes: * The alloc-32.fir test is now marked as requiring the X86 target. * Drive-by fixes uncovered when fixing tests involving malloc
I came across @georgestagg's work on getting Flang to target WebAssembly and attempted to clean up the hacks so that it can actually be upstreamed. This patch fixes dynamic allocations, and I am submitting it first as it is less invasive than the work needed to fix RTBuilder.
This change first ensures that the MLIR->LLVM lowering pass's
LLVMTypeConverteractually has access to the real target DataLayout, as it was previously making do with a default one. This only seems to affect the determination of the target's pointer size (I assume everything else is using the MLIR DataLayout?). The AllocMemOp lowering then inserts a type conversion if the target has 32-bit pointers. This assumes thatsize_tis the same size as pointers.I haven't checked whether anything else in CodeGen needs changing for cross-compiling (especially since I'm not actually familiar with Fortran), but this change should make any necessary future fixes easier.