[flang-commits] [flang] [flang] Do not hoist fir.field_index out of loops (PR #222891)

[via flang-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Kareem Ergawy (ergawy)

<details>
<summary>Changes</summary>

Lowering a consumer of a !fir.field value inspects its defining operation: for a record whose layout is known at compile time the field becomes an LLVM GEP struct index, which must be a constant, and otherwise the `field` attribute is read off the defining op. A field value therefore may not be a block argument.

LICM broke that. fir.field_index is Pure and takes no operands, so it is trivially loop-invariant and was hoisted out of the loop. Lowering emits one inside each arm of a construct -- for example the CASEs of a SELECT CASE that each pass a different component of the same derived type as an actual argument -- so hoisting them left those arms as otherwise-identical blocks differing only in that operand. Block merging then merged them and threaded the field through a new block argument, and codegen aborted with "must be a constant".

Leave producers of a !fir.field where they are. The arms then differ by an operation rather than by an operand, so they are no longer merge candidates. Consumers are unaffected and still hoist: fir.coordinate_of uses the attribute form of the field, so it does not depend on the fir.field_index value.

The check keys on the result type rather than on FieldIndexOp so that any operation producing a !fir.field is covered, and sits in the predicate shared by the direct-loop and nested-region hoisting paths.

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


2 Files Affected:

- (modified) flang/lib/Optimizer/Transforms/LoopInvariantCodeMotion.cpp (+15) 
- (modified) flang/test/Transforms/licm.fir (+10-2) 


``````````diff
diff --git a/flang/lib/Optimizer/Transforms/LoopInvariantCodeMotion.cpp b/flang/lib/Optimizer/Transforms/LoopInvariantCodeMotion.cpp
index 97958003ddfd2..4003f1cb30b18 100644
--- a/flang/lib/Optimizer/Transforms/LoopInvariantCodeMotion.cpp
+++ b/flang/lib/Optimizer/Transforms/LoopInvariantCodeMotion.cpp
@@ -321,6 +321,21 @@ void LoopInvariantCodeMotion::runOnOperation() {
   std::function<bool(Operation *, LoopLikeOpInterface, bool)>
       shouldMoveOutOfLoop = [&](Operation *op, LoopLikeOpInterface loopLike,
                                 bool maybeConditionallyExecuted) {
+        // Never hoist a producer of a !fir.field. Lowering a consumer of a
+        // field value inspects its defining operation: for a record whose
+        // layout is known at compile time the field becomes an LLVM GEP struct
+        // index, which must be a constant. Hoisting fir.field_index out of the
+        // arms of a construct (e.g. the CASEs of a SELECT CASE, each passing a
+        // different component of the same derived type) leaves those arms as
+        // otherwise-identical blocks differing only in this operand, which lets
+        // block merging thread it through a new block argument -- destroying
+        // the defining operation that codegen needs.
+        if (llvm::any_of(op->getResultTypes(),
+                         [](mlir::Type t) { return isa<fir::FieldType>(t); })) {
+          LDBG() << "Not hoisting producer of a field value: " << *op;
+          return false;
+        }
+
         if (isPure(op)) {
           LDBG() << "Pure operation: " << *op;
           return true;
diff --git a/flang/test/Transforms/licm.fir b/flang/test/Transforms/licm.fir
index b695ea2db184f..b841c5e93cb06 100644
--- a/flang/test/Transforms/licm.fir
+++ b/flang/test/Transforms/licm.fir
@@ -2015,13 +2015,20 @@ func.func @test_acc_loop_private2_hoisting() {
 }
 
 // -----
-// Test hoisting of fir.field_index and fir.coordinate_of.
+// Test hoisting of fir.coordinate_of.
+// fir.field_index is deliberately NOT hoisted: lowering a consumer of a field
+// value inspects its defining operation (for a record whose layout is known at
+// compile time the field becomes a constant LLVM GEP struct index). Hoisting it
+// out of the arms of a construct leaves otherwise-identical blocks differing
+// only in that operand, which lets block merging thread it through a block
+// argument and destroy the defining operation codegen needs. Its consumers are
+// unaffected and still hoist -- fir.coordinate_of below uses the attribute form
+// of the field, so it does not depend on the fir.field_index value.
 // CHECK-LABEL:   func.func @_QMmPtest(
 // CHECK-SAME:      %[[ARG0:[0-9]+|[a-zA-Z$._-][a-zA-Z0-9$._-]*]]: !fir.ref<i32> {fir.bindc_name = "n"}) {
 // CHECK:           %[[DUMMY_SCOPE_0:.*]] = fir.dummy_scope : !fir.dscope
 // CHECK:           %[[ADDRESS_OF_0:.*]] = fir.address_of(@_QMmEglob) : !fir.ref<!fir.type<_QMmTt{a:!fir.box<!fir.heap<!fir.array<?xf32>>>}>>
 // CHECK:           %[[DECLARE_0:.*]] = fir.declare %[[ADDRESS_OF_0]] {uniq_name = "_QMmEglob"} : (!fir.ref<!fir.type<_QMmTt{a:!fir.box<!fir.heap<!fir.array<?xf32>>>}>>) -> !fir.ref<!fir.type<_QMmTt{a:!fir.box<!fir.heap<!fir.array<?xf32>>>}>>
-// CHECK:           %[[FIELD_INDEX_0:.*]] = fir.field_index a, !fir.type<_QMmTt{a:!fir.box<!fir.heap<!fir.array<?xf32>>>}>
 // CHECK:           %[[COORDINATE_OF_0:.*]] = fir.coordinate_of %[[DECLARE_0]], a : (!fir.ref<!fir.type<_QMmTt{a:!fir.box<!fir.heap<!fir.array<?xf32>>>}>>) -> !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
 // CHECK:           %[[LOAD_1:.*]] = fir.load %[[COORDINATE_OF_0]] : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
 // CHECK:           %[[BOX_ADDR_0:.*]] = fir.box_addr %[[LOAD_1]] : (!fir.box<!fir.heap<!fir.array<?xf32>>>) -> !fir.heap<!fir.array<?xf32>>
@@ -2029,6 +2036,7 @@ func.func @test_acc_loop_private2_hoisting() {
 // CHECK:           %[[BOX_DIMS_0:.*]]:3 = fir.box_dims %[[LOAD_1]], %[[CONSTANT_2]] : (!fir.box<!fir.heap<!fir.array<?xf32>>>, index) -> (index, index, index)
 // CHECK:           %[[SHAPE_SHIFT_0:.*]] = fir.shape_shift %[[BOX_DIMS_0]]#0, %[[BOX_DIMS_0]]#1 : (index, index) -> !fir.shapeshift<1>
 // CHECK:           %[[DO_LOOP_0:.*]] = fir.do_loop
+// CHECK:             %[[FIELD_INDEX_0:.*]] = fir.field_index a, !fir.type<_QMmTt{a:!fir.box<!fir.heap<!fir.array<?xf32>>>}>
 func.func @_QMmPtest(%arg0: !fir.ref<i32> {fir.bindc_name = "n"}) {
   %cst = arith.constant 1.000000e+00 : f32
   %c1 = arith.constant 1 : index

``````````

</details>


https://github.com/llvm/llvm-project/pull/222891