[flang][CodeGen] Transform `fir.{store|load}` to `llvm.{store|load}`

[awarzynski]

Summary

This patch extends the FIRToLLVMLowering pass in Flang by adding a
hook to transform fir.store/fir.load to llvm.store/fir.load,
respectively.

This is part of the upstreaming effort from the fir-dev branch in [1].

[1] https://github.com/flang-compiler/f18-llvm-project

Patch originally written by:
Co-authored-by: Eric Schweitz <[e-mail]>
Co-authored-by: Jean Perier <[e-mail]>

[awarzynski]

I wanted to add a test for fir.ref<fir.box>, but that does not work yet (it works fine on fir-dev). I wasn't sure why, so my comment in "flang/test/Fir/convert-to-llvm.fir" is a bit generic. Any pointers? Thanks!

[clementval]

In D113090#3105376, @awarzynski wrote:

I wanted to add a test for fir.ref<fir.box>, but that does not work yet (it works fine on fir-dev). I wasn't sure why, so my comment in "flang/test/Fir/convert-to-llvm.fir" is a bit generic. Any pointers? Thanks!

Not all of the type conversions have been upstreamed. I usually upstream the one I need with the op conversion I'm upstreaming.

flang/lib/Optimizer/CodeGen/TypeConverter.h

[clementval]

Test is failing on the pre-check.

[clementval]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

175

Can you address this formatting issue?

[kiranchandramohan]

In D113090#3105389, @clementval wrote:

In D113090#3105376, @awarzynski wrote:

I wanted to add a test for fir.ref<fir.box>, but that does not work yet (it works fine on fir-dev). I wasn't sure why, so my comment in "flang/test/Fir/convert-to-llvm.fir" is a bit generic. Any pointers? Thanks!

Not all of the type conversions have been upstreamed. I usually upstream the one I need with the op conversion I'm upstreaming.

flang/lib/Optimizer/CodeGen/TypeConverter.h

So does @awarzynski have to bring in the convertBoxType function?

flang/test/Fir/convert-to-llvm.fir

153

missed the parameters.
Would it be good to match the parameters and the store operands?

174

missed the parameters.
Would it be good to match the parameters and the load operands?

[clementval]

In D113090#3105615, @kiranchandramohan wrote:

In D113090#3105389, @clementval wrote:

In D113090#3105376, @awarzynski wrote:

I wanted to add a test for fir.ref<fir.box>, but that does not work yet (it works fine on fir-dev). I wasn't sure why, so my comment in "flang/test/Fir/convert-to-llvm.fir" is a bit generic. Any pointers? Thanks!

Not all of the type conversions have been upstreamed. I usually upstream the one I need with the op conversion I'm upstreaming.

flang/lib/Optimizer/CodeGen/TypeConverter.h

So does @awarzynski have to bring in the convertBoxType function?

Or it will come with D112961 if anyone review it.

[clementval]

flang/test/Fir/convert-to-llvm.fir

153

+1

[mehdi_amini]

flang/test/Fir/convert-to-llvm.fir

156

In general: please use CHECK-LABEL for delimiting the test (that helps FileCheck, in particular the print in case of failure is much friendlier) and minimize the checks, while capturing what matters (here I suspect types and argument flowing in the right place it what you're checking (I think that's also what Kiran hinted at):

// CHECK-LABEL: llvm.func @test_store
// CHECK-SAME: %[[ARG0:.*]]: <type here>, %[[ARG1:.*]]: <type here>
// CHECK:    llvm.store %[[ARG0]], .....

[awarzynski]

In D113090#3105647, @clementval wrote:

In D113090#3105615, @kiranchandramohan wrote:

So does @awarzynski have to bring in the convertBoxType function?

Or it will come with D112961 if anyone review it.

Tl;Dr Perhaps we should just add more tests later?

Longer version - I've tried applying D112961, but that didn't work. It depends on 2 other patches, so perhaps I wasn't applying it correctly? I've just tried my usual arc patch D112961. Regardless, if we prefer to have all tests added in one patch then I can set the dependencies accordingly and re-visit arc patch.

[awarzynski]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

175

Sorry about that :/

flang/test/Fir/convert-to-llvm.fir

156

haha, as I was in the process of changing that :-)

[awarzynski]

Address PR comments (refine tests, fix formatting)

[clementval]

LGTM

[awarzynski]

Rebased, addes tests for fir.box. This has already been accepted by @clementval, so I will merge this if there are no new comments.

[clementval]

Still LGTM

flang/lib/Optimizer/CodeGen/CodeGen.cpp

 #include "PassDetail.h"
 #include "flang/Optimizer/Dialect/FIROps.h"
 #include "flang/Optimizer/Dialect/FIRType.h"
 #include "mlir/Conversion/ArithmeticToLLVM/ArithmeticToLLVM.h"
 #include "mlir/Conversion/LLVMCommon/Pattern.h"
 #include "mlir/Conversion/LLVMCommon/TypeConverter.h"
 #include "mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h"
 #include "mlir/IR/BuiltinTypes.h"
+#include "mlir/IR/Location.h"
 #include "mlir/Pass/Pass.h"
 #include "llvm/ADT/ArrayRef.h"
 
 #define DEBUG_TYPE "flang-codegen"
 
 // fir::LLVMTypeConverter for converting to LLVM IR dialect types.
 #include "TypeConverter.h"
 
if (optLinkage.hasValue()) {
         return mlir::LLVM::Linkage::Common;
       if (name == "weak")
         return mlir::LLVM::Linkage::Weak;
     }
     return mlir::LLVM::Linkage::External;
   }
 };
 
+// convert to LLVM IR dialect `load`
+struct LoadOpConversion : public FIROpConversion<fir::LoadOp> {
+  using FIROpConversion::FIROpConversion;
+
+  mlir::LogicalResult
+  matchAndRewrite(fir::LoadOp load, OpAdaptor adaptor,
+                  mlir::ConversionPatternRewriter &rewriter) const override {
+    // fir.box is a special case because it is considered as an ssa values in
+    // fir, but it is lowered as a pointer to a descriptor. So fir.ref<fir.box>
+    // and fir.box end up being the same llvm types and loading a
+    // fir.ref<fir.box> is actually a no op in LLVM.
+    if (load.getType().isa<fir::BoxType>()) {
+      rewriter.replaceOp(load, adaptor.getOperands()[0]);
+    } else {
+      mlir::Type ty = convertType(load.getType());
+      ArrayRef<NamedAttribute> at = load->getAttrs();
+      rewriter.replaceOpWithNewOp<mlir::LLVM::LoadOp>(
+          load, ty, adaptor.getOperands(), at);
+    }
+    return success();
+  }
+};
+
+// convert to LLVM IR dialect `store`
+struct StoreOpConversion : public FIROpConversion<fir::StoreOp> {
+  using FIROpConversion::FIROpConversion;
+
+  mlir::LogicalResult
+  matchAndRewrite(fir::StoreOp store, OpAdaptor adaptor,
+                  mlir::ConversionPatternRewriter &rewriter) const override {
+    if (store.value().getType().isa<fir::BoxType>()) {
+      // fir.box value is actually in memory, load it first before storing it.
+      mlir::Location loc = store.getLoc();
+      mlir::Type boxPtrTy = adaptor.getOperands()[0].getType();
+      auto val = rewriter.create<mlir::LLVM::LoadOp>(
+          loc, boxPtrTy.cast<mlir::LLVM::LLVMPointerType>().getElementType(),
+          adaptor.getOperands()[0]);
+      rewriter.replaceOpWithNewOp<mlir::LLVM::StoreOp>(
+          store, val, adaptor.getOperands()[1]);
+    } else {
+      rewriter.replaceOpWithNewOp<mlir::LLVM::StoreOp>(
+          store, adaptor.getOperands()[0], adaptor.getOperands()[1]);
+    }
+    return success();
+  }
+};
+
 // convert to LLVM IR dialect `undef`
 struct UndefOpConversion : public FIROpConversion<fir::UndefOp> {
   using FIROpConversion::FIROpConversion;
 
   mlir::LogicalResult
   matchAndRewrite(fir::UndefOp undef, OpAdaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
     rewriter.replaceOpWithNewOp<mlir::LLVM::UndefOp>(

 class FIRToLLVMLowering : public fir::FIRToLLVMLoweringBase<FIRToLLVMLowering> {
 public:
   mlir::ModuleOp getModule() { return getOperation(); }
 
   void runOnOperation() override final {
     auto *context = getModule().getContext();
     fir::LLVMTypeConverter typeConverter{getModule()};
     mlir::OwningRewritePatternList pattern(context);
-    pattern
-        .insert<AddrOfOpConversion, HasValueOpConversion, GlobalOpConversion,
-                UndefOpConversion, UnreachableOpConversion, ZeroOpConversion>(
-            typeConverter);
+    pattern.insert<AddrOfOpConversion, HasValueOpConversion, GlobalOpConversion,
+                   LoadOpConversion, StoreOpConversion, UndefOpConversion,
+                   UnreachableOpConversion, ZeroOpConversion>(typeConverter);
     mlir::populateStdToLLVMConversionPatterns(typeConverter, pattern);
     mlir::arith::populateArithmeticToLLVMConversionPatterns(typeConverter,
                                                             pattern);
     mlir::ConversionTarget target{*context};
     target.addLegalDialect<mlir::LLVM::LLVMDialect>();
 
     // required NOPs for applying a full conversion
     target.addLegalOp<mlir::ModuleOp>();

flang/test/Fir/convert-to-llvm.fir

 
 // CHECK:  llvm.func @test_unreachable() {
 // CHECK-NEXT:    llvm.unreachable
 // CHECK-NEXT:  }
 
 func @test_unreachable() {
   fir.unreachable
 }
+
+// -----
+
+// Verify that fir.store is transformed to llvm.store
+
+// CHECK-LABEL:   llvm.func @test.store(
+// CHECK-SAME:    %[[arg0:.*]]: i64, %[[arg1:.*]]: !llvm.ptr<i64>) {
+// CHECK-NEXT:    llvm.store %[[arg0]], %[[arg1]] : !llvm.ptr<i64>
+// CHECK-NEXT:    llvm.return
+// CHECK-NEXT:  }
+
+func @test.store(%lb : index, %addr : !fir.ref<index>) {
+  fir.store %lb to %addr : !fir.ref<index>
+  return
+}
+
+// TODO Add a test with fir.ref<fir.box> once we have the required conversions
+// are avaialble, e.g.
+// func @test_store_box(%array : !fir.ref<!fir.box<!fir.array<?x?xf32>>>, %box : !fir.box<!fir.array<?x?xf32>>) {
+//   fir.store %box to %array : !fir.ref<!fir.box<!fir.array<?x?xf32>>>
+//   return
+// }
+
+// -----
+
+// Verify that fir.load is transformed to llvm.load
+
+// CHECK-LABEL: llvm.func @test_load(
+// CHECK-SAME:  %[[arg1:.*]]: !llvm.ptr<i64>) {
+// CHECK-NEXT:    %0 = llvm.load %[[arg1]] : !llvm.ptr<i64>
+// CHECK-NEXT:    llvm.return
+// CHECK-NEXT:  }
+
+func @test_load(%addr : !fir.ref<index>) {
+  %0 = fir.load %addr : !fir.ref<index>
+  return
+}
+
+// TODO Add a test with fir.ref<fir.box> once the required conversions are
+// available.