[flang][hlfir] Do not emit extra declare for dummy used in BLOCK

[jeanPerier]

When a variable is used in a specification expression in a scope, it is added to the list of variables that must be instantiated when lowering the scope. When lowering a BLOCK, this caused instantiateVar to be called again on all the host block variables appearing in block variable specification expressions. This caused an extra declare to be emitted for dummy inside block (for non dummy, instantiateVar is a no-op if the symbol is already mapped).

Only call instantiateVar if the symbol is not mapped when lowering BLOCK variables.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

Author: None (jeanPerier)

Changes

When a variable is used in a specification expression in a scope, it is added to the list of variables that must be instantiated when lowering the scope. When lowering a BLOCK, this caused instantiateVar to be called again on all the host block variables appearing in block variable specification expressions. This caused an extra declare to be emitted for dummy inside block (for non dummy, instantiateVar is a no-op if the symbol is already mapped).

Only call instantiateVar if the symbol is not mapped when lowering BLOCK variables.

---

Full diff: https://github.com/llvm/llvm-project/pull/69184.diff

2 Files Affected:

• (modified) flang/lib/Lower/Bridge.cpp (+6-2)
• (added) flang/test/Lower/HLFIR/convert-variable-block.f90 (+25)

diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index 5ac4d822faaae58..e6d6a19009e9de6 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -2610,8 +2610,12 @@ class FirConverter : public Fortran::lower::AbstractConverter {
         scopeBlockIdMap.try_emplace(&scope, ++blockId);
         Fortran::lower::AggregateStoreMap storeMap;
         for (const Fortran::lower::pft::Variable &var :
-             Fortran::lower::pft::getScopeVariableList(scope))
-          instantiateVar(var, storeMap);
+             Fortran::lower::pft::getScopeVariableList(scope)) {
+          // Do no instantiate again variables from the block host
+          // that appears in specification of block variables.
+          if (!var.hasSymbol() || !lookupSymbol(var.getSymbol()))
+            instantiateVar(var, storeMap);
+        }
       } else if (e.getIf<Fortran::parser::EndBlockStmt>()) {
         if (eval.lowerAsUnstructured())
           maybeStartBlock(e.block);
diff --git a/flang/test/Lower/HLFIR/convert-variable-block.f90 b/flang/test/Lower/HLFIR/convert-variable-block.f90
new file mode 100644
index 000000000000000..30f8eacaaed17bd
--- /dev/null
+++ b/flang/test/Lower/HLFIR/convert-variable-block.f90
@@ -0,0 +1,25 @@
+! Test that hlfir.declare is not created again for dummy arguments
+! used in specifications of BLOCK variables.
+! RUN: bbc -emit-hlfir %s -o - | FileCheck %s
+
+subroutine test(n)
+  integer(8) :: n
+  call before_block()
+  block
+    real :: x(n)
+    call foo(x)
+  end block
+end subroutine
+! CHECK-LABEL:   func.func @_QPtest(
+! CHECK-SAME:                       %[[VAL_0:.*]]: !fir.ref<i64> {fir.bindc_name = "n"}) {
+! CHECK:           %[[VAL_1:.*]]:2 = hlfir.declare %[[VAL_0]] {uniq_name = "_QFtestEn"} : (!fir.ref<i64>) -> (!fir.ref<i64>, !fir.ref<i64>)
+! CHECK:           fir.call @_QPbefore_block() {{.*}}: () -> ()
+! CHECK:           %[[VAL_3:.*]] = fir.load %[[VAL_1]]#0 : !fir.ref<i64>
+! CHECK:           %[[VAL_4:.*]] = fir.convert %[[VAL_3]] : (i64) -> index
+! CHECK:           %[[VAL_5:.*]] = arith.constant 0 : index
+! CHECK:           %[[VAL_6:.*]] = arith.cmpi sgt, %[[VAL_4]], %[[VAL_5]] : index
+! CHECK:           %[[VAL_7:.*]] = arith.select %[[VAL_6]], %[[VAL_4]], %[[VAL_5]] : index
+! CHECK:           %[[VAL_8:.*]] = fir.alloca !fir.array<?xf32>, %[[VAL_7]] {bindc_name = "x", uniq_name = "_QFtestB1Ex"}
+! CHECK:           %[[VAL_9:.*]] = fir.shape %[[VAL_7]] : (index) -> !fir.shape<1>
+! CHECK:           %[[VAL_10:.*]]:2 = hlfir.declare %[[VAL_8]](%[[VAL_9]]) {uniq_name = "_QFtestB1Ex"} : (!fir.ref<!fir.array<?xf32>>, !fir.shape<1>) -> (!fir.box<!fir.array<?xf32>>, !fir.ref<!fir.array<?xf32>>)
+! CHECK:           fir.call @_QPfoo(%[[VAL_10]]#1) {{.*}}: (!fir.ref<!fir.array<?xf32>>) -> ()

[vzakhari]

Thank you, Jean!

[vdonaldson]

Would it be possible to do this earlier, something like this:

+++ b/flang/lib/Lower/PFTBuilder.cpp
@@ -1474,10 +1474,12 @@ private:
   // other symbols.
   int analyze(const semantics::Symbol &sym) {
     auto done = seen.insert(&sym);
     if (!done.second)
       return 0;
+    if (lookupSymbol(sym))
+      return 0; // sym is from a containing scope
     LLVM_DEBUG(llvm::dbgs() << "analyze symbol " << &sym << " in <"
                             << &sym.owner() << ">: " << sym << '\n');
     const bool isProcedurePointerOrDummy =
         semantics::IsProcedurePointer(sym) ||
         (semantics::IsProcedure(sym) && IsDummy(sym));

[jeanPerier]

Would it be possible to do this earlier, something like this:

Not currently because this analysis does not have access to the current symbol mapping. From a library/tools perspective, I would rather avoid having the PFT tools behavior depending on the current state of the Bridge (that comes afterwards).

@vdonaldson, I can move it into instantiateVar here though if I propagate some flag that instantiated variable should be skipped. Would that be better?

[vdonaldson]

I can move it into instantiateVar. Would that be better?

Probably yes, if the propagation code is fairly simple.

But I'm still inclined to think it would be better though to do this earlier if that is reasonable, which it might or might not be. One idea there is to explicitly save the incoming scope in the two SymbolDependenceAnalysis constructors and then in analyze have an early exit if the scope of the symbol being analyzed does not match that scope. I'm not completely sure if that would be ok though.

[jeanPerier]

One idea there is to explicitly save the incoming scope in the two SymbolDependenceAnalysis constructors and then in analyze have an early exit if the scope of the symbol being analyzed does not match that scope. I'm not completely sure if that would be ok though.

That would at likely not work with used module symbols/host associated variables whose scope may differ from the one being analyzed. I am not saying that what you are suggesting is not possible, but I would rather avoid making the PFT analysis more complex if possible. The gain seems small to me unless you have a program with thousands of host variables being used in block variable specifications.

The nicest fix should be to have instantiateVariable rely on something else to propagate the IR dummy/results and always refuse to instantiate again symbols. But it is too much refactoring for what needs to be fixed here IMHO.

[vdonaldson]

A simple update to SymbolDependenceAnalysis to compare scope s works with the vast majority of tests because module and host syms have local syms that provide indirect access to their ultimate syms.

However, the scope comparison causes lit tests explicit-interface-results and module-single-point-of-def to fail due to atypical module variable accesses - for a function result in the former case; and for a namelist reference in the latter case.

So I'm ok with either the original change, or your possible alternate fix that moves the check near the beginning of instantiateVar if that one turns out to be better.

Thanks for the fix.

[jeanPerier]

Thanks for the review Val! I will keep it as it is. Threading the boolean makes things a bit harder to read in instantiateVar.