Fix further review comments.

Additionally fixed a bug where same argument used twice would not have the
store and alloca removed.

Add test to check for those instructions being removed.

Author: Leporacanthicus

flang/lib/Optimizer/Transforms/ConstantArgumentGlobalisation.cpp

@@ -22,7 +22,7 @@ namespace fir {
 #include "flang/Optimizer/Transforms/Passes.h.inc"
 } // namespace fir
 
-#define DEBUG_TYPE "flang-constang-argument-globalisation-opt"
+#define DEBUG_TYPE "flang-constant-argument-globalisation-opt"
 
 namespace {
 unsigned uniqueLitId = 1;
@@ -45,7 +45,7 @@ class CallOpRewriter : public mlir::OpRewritePattern<fir::CallOp> {
     bool needUpdate = false;
     fir::FirOpBuilder builder(rewriter, module);
     llvm::SmallVector<mlir::Value> newOperands;
-    llvm::SmallVector<mlir::Operation *> toErase;
+    llvm::SmallVector<std::pair<mlir::Operation *, mlir::Operation *>> allocas;
     for (const mlir::Value &a : callOp.getArgs()) {
       auto alloca = mlir::dyn_cast_or_null<fir::AllocaOp>(a.getDefiningOp());
       // We can convert arguments that are alloca, and that has
@@ -74,53 +74,45 @@ class CallOpRewriter : public mlir::OpRewritePattern<fir::CallOp> {
         }
       }
 
-      // If we didn't find one signle store, add argument as is, and move on.
+      // If we didn't find any store, or multiple stores, add argument as is
+      // and move on.
       if (!store) {
         newOperands.push_back(a);
         continue;
       }
 
       LLVM_DEBUG(llvm::dbgs() << " found store " << *store << "\n");
 
-      mlir::Operation *constant_def = store->getOperand(0).getDefiningOp();
-      // Expect constant definition operation or force legalisation of the
-      // callOp and continue with its next argument
-      if (!mlir::isa<mlir::arith::ConstantOp>(constant_def)) {
+      mlir::Operation *definingOp = store->getOperand(0).getDefiningOp();
+      // If not a constant, add to operands and move on.
+      if (!mlir::isa<mlir::arith::ConstantOp>(definingOp)) {
         // Unable to remove alloca arg
         newOperands.push_back(a);
         continue;
       }
 
-      LLVM_DEBUG(llvm::dbgs() << " found define " << *constant_def << "\n");
+      LLVM_DEBUG(llvm::dbgs() << " found define " << *definingOp << "\n");
 
       std::string globalName =
           "_global_const_." + std::to_string(uniqueLitId++);
       assert(!builder.getNamedGlobal(globalName) &&
              "We should have a unique name here");
 
-      unsigned count = 0;
-      for (mlir::Operation *s : alloca->getUsers())
-        if (di.dominates(store, s))
-          ++count;
-
-      // Delete if dominates itself and one more operation (which should
-      // be callOp)
-      if (count == 2)
-        toErase.push_back(store);
+      allocas.push_back(std::make_pair(alloca, store));
 
       auto loc = callOp.getLoc();
       fir::GlobalOp global = builder.createGlobalConstant(
           loc, varTy, globalName,
           [&](fir::FirOpBuilder &builder) {
-            mlir::Operation *cln = constant_def->clone();
+            mlir::Operation *cln = definingOp->clone();
             builder.insert(cln);
             mlir::Value val =
                 builder.createConvert(loc, varTy, cln->getResult(0));
             builder.create<fir::HasValueOp>(loc, val);
           },
           builder.createInternalLinkage());
-      mlir::Value addr = {builder.create<fir::AddrOfOp>(
-          loc, global.resultType(), global.getSymbol())};
+      mlir::Value addr = builder.create<fir::AddrOfOp>(
+          loc, global.resultType(), global.getSymbol());
       newOperands.push_back(addr);
       needUpdate = true;
     }
@@ -139,8 +131,19 @@ class CallOpRewriter : public mlir::OpRewritePattern<fir::CallOp> {
       newOp->setAttrs(callOp->getAttrs());
       rewriter.replaceOp(callOp, newOp);
 
-      for (auto e : toErase)
-        rewriter.eraseOp(e);
+      for (auto a : allocas) {
+	unsigned count = 0;
+      
+	for (auto i : a.first->getUsers())
+	  ++count;
+
+	// If the alloca is only used for a store and the call operand, the
+	// store is no longer required.
+	if (count == 1) {
+	  rewriter.eraseOp(a.second);
+	  rewriter.eraseOp(a.first);
+	}
+      }
       LLVM_DEBUG(llvm::dbgs() << "global constant for " << callOp << " as "
                               << newOp << '\n');
       return mlir::success();

flang/test/Transforms/constant-argument-globalisation-2.fir

@@ -78,3 +78,21 @@ module {
 // CHECK: }
 
 }
+
+// -----
+// Check that same argument used twice is converted.
+module {
+  func.func @func(%arg0: !fir.ref<i32>, %arg1: i1) {
+    %c2_i32 = arith.constant 2 : i32
+    %addr1 = fir.alloca i32 {adapt.valuebyref}
+    fir.store %c2_i32 to %addr1 : !fir.ref<i32>
+    fir.call @sub1(%addr1, %addr1) : (!fir.ref<i32>, !fir.ref<i32>) -> ()
+    return
+  }
+}
+
+// CHECK-LABEL: func.func @func
+// CHECK-NEXT: %[[ARG1:.*]] = fir.address_of([[CONST1:@.*]]) : !fir.ref<i32>
+// CHECK-NEXT: %[[ARG2:.*]] = fir.address_of([[CONST2:@.*]]) : !fir.ref<i32>
+// CHECK-NEXT: fir.call @sub1(%[[ARG1]], %[[ARG2]])
+// CHECK-NEXT: return