Fix loop_annotation name in unit tests

[fiigii]

The attribute name is loop_annotation instead of llvm.loop_annotation.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Fei Peng (fiigii)

Changes

The attribute name is loop_annotation instead of llvm.loop_annotation.

---

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

1 Files Affected:

• (modified) mlir/test/Conversion/SCFToControlFlow/convert-to-cfg.mlir (+8-8)

diff --git a/mlir/test/Conversion/SCFToControlFlow/convert-to-cfg.mlir b/mlir/test/Conversion/SCFToControlFlow/convert-to-cfg.mlir
index 0c4f20e8d1a04..d697acfccb2e5 100644
--- a/mlir/test/Conversion/SCFToControlFlow/convert-to-cfg.mlir
+++ b/mlir/test/Conversion/SCFToControlFlow/convert-to-cfg.mlir
@@ -710,9 +710,9 @@ func.func @forall(%num_threads: index) {
 //      CHECK: ^[[bb3]](%{{.*}}: index):
 //      CHECK:   cf.cond_br %{{.*}}, ^[[bb4:.*]], ^[[bb5:.*]]
 //      CHECK: ^[[bb4]]:
-//      CHECK:   cf.br ^[[bb3]]({{.*}}) {llvm.loop_annotation = #[[FULL_UNROLL]]}
+//      CHECK:   cf.br ^[[bb3]]({{.*}}) {loop_annotation = #[[FULL_UNROLL]]}
 //      CHECK: ^[[bb5]]:
-//      CHECK:   cf.br ^[[bb1]]({{.*}}) {llvm.loop_annotation = #[[NO_UNROLL]]}
+//      CHECK:   cf.br ^[[bb1]]({{.*}}) {loop_annotation = #[[NO_UNROLL]]}
 //      CHECK: ^[[bb6]]:
 //      CHECK:   return
 #no_unroll = #llvm.loop_annotation<unroll = <disable = true>>
@@ -724,8 +724,8 @@ func.func @simple_std_for_loops_annotation(%arg0 : index, %arg1 : index, %arg2 :
     %c4 = arith.constant 4 : index
     scf.for %i1 = %c0 to %c4 step %c1 {
       %c1_0 = arith.constant 1 : index
-    } {llvm.loop_annotation = #full_unroll}
-  } {llvm.loop_annotation = #no_unroll}
+    } {loop_annotation = #full_unroll}
+  } {loop_annotation = #no_unroll}
   return
 }
 
@@ -735,7 +735,7 @@ func.func @simple_std_for_loops_annotation(%arg0 : index, %arg1 : index, %arg2 :
 // CHECK: #[[NO_UNROLL:.*]] = #llvm.loop_annotation<unroll = #[[LOOP_UNROLL_DISABLE]]>
 // CHECK: func @simple_while_loops_annotation
 //      CHECK: cf.br
-//      CHECK: cf.cond_br {{.*}} {llvm.loop_annotation = #[[NO_UNROLL]]}
+//      CHECK: cf.cond_br {{.*}} {loop_annotation = #[[NO_UNROLL]]}
 //      CHECK: return
 #no_unroll = #llvm.loop_annotation<unroll = <disable = true>>
 func.func @simple_while_loops_annotation(%arg0 : i1) {
@@ -743,7 +743,7 @@ func.func @simple_while_loops_annotation(%arg0 : i1) {
     scf.condition(%arg0)
   } do {
     scf.yield
-  } attributes {llvm.loop_annotation = #no_unroll}
+  } attributes {loop_annotation = #no_unroll}
   return
 }
 
@@ -754,7 +754,7 @@ func.func @simple_while_loops_annotation(%arg0 : i1) {
 // CHECK: func @do_while_loops_annotation
 // CHECK: cf.br
 // CHECK: cf.cond_br
-// CHECK: cf.br {{.*}} {llvm.loop_annotation = #[[NO_UNROLL]]}
+// CHECK: cf.br {{.*}} {loop_annotation = #[[NO_UNROLL]]}
 // CHECK: return
 #no_unroll = #llvm.loop_annotation<unroll = <disable = true>>
 func.func @do_while_loops_annotation() {
@@ -765,7 +765,7 @@ func.func @do_while_loops_annotation() {
   } do {
  ^bb0(%arg2: i32):    
     scf.yield %c0_i32: i32
-  } attributes {llvm.loop_annotation = #no_unroll}
+  } attributes {loop_annotation = #no_unroll}
   return
 }
 

[xiaoleis-nv]

This is a valid update. I do not have permission to approve this PR; perhaps others can assist.

[ftynse]

What problem does this solve?

[fiigii]

What problem does this solve?

This is a confusing test. I was trying to control loop unrolling at the MLIR level and was confused by the distinction between llvm.loop_annotation and loop_annotation. Finally, I realized that mlir-translate does not recognize llvm.loop_annotation and won't lower it to LLVM IR. So, this PR fixes the incorrect usage of loop annotations.

Additionally, I hope MLIR clarifies it in the language reference.

[joker-eph]

I think we have a more fundamental problem here where this all works "by accident".

The llvm. prefix for discardable attributes is in theory necessary: discardable attributes should have a dialect prefix (we just never wrote the verifier for this :( ).

However the llvm.br operation has a inherent attribute named loop_annotation.

It turns out that we copy LLVM dialect attributes when lowering SCF to CF:

static void propagateLoopAttrs(Operation *scfOp, Operation *brOp) {
  // Let the CondBranchOp carry the LLVM attributes from the ForOp, such as the
  // llvm.loop_annotation attribute.
  // LLVM requires the loop metadata to be attached on the "latch" block. Which
  // is the back-edge to the header block (conditionBlock)
  SmallVector<NamedAttribute> llvmAttrs;
  llvm::copy_if(scfOp->getAttrs(), std::back_inserter(llvmAttrs),
                [](auto attr) {
                  return isa<LLVM::LLVMDialect>(attr.getValue().getDialect());
                });
  brOp->setDiscardableAttrs(llvmAttrs);
}

However here we check just for the dialect of the attribute (value) and not the key, so we propagate a discardable attribute whose key just collide with the inherent attribute of the LLVM branch.
This is all broken :)

[fiigii]

Agree. I was also confused by the code above, and intentionally processing LLVM dialect in SCF/CF dialects seems not very reasonable.

I am not very familiar with MLIR, based on my experience in handling LLVM metadata, we may need a more generic and clearer approach, for example:

• During pure IR conversion like SCF->CF, unconditionally copy all the attributes.
• During transformation or optimizations, retrieve non-discardable attributes by the attribute names, and process (change, preserve, or drop) the attributes according to the transformation semantics. As I understand, "discardable attribute" is similar to LLVM's metadata, which we should keep them as much as possible, but it's also okay if they are lost by accident.