[PATCH] D111288: [fir] Add data flow optimization pass

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski accepted this revision.
awarzynski added a comment.

Thanks for trimming the tests @clementval - now it is much clearer what is being tested! I think the the following pattern:

  // CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
  // CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>

can be reduced to:

  // CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>

Not a blocker for me. I've also left a few inline comments, but mostly nits! LGTM (modulo the typos), but please wait for Mehdi to take a look before merging.

Thanks for working on this!



================
Comment at: flang/test/Fir/memref-data-flow.fir:57
+
+// All store-load chains are removed
+
----------------
[nit] This comment applies to all cases in this file - I'd move it to the very top (near the `RUN` line). You may also want to note that `fir.store`s and `fir.load`s that are not in a chain are preserved. Perhaps that's obvious?


================
Comment at: flang/test/Fir/memref-data-flow.fir:75-76
+// CHECK:         %{{.*}} = fir.convert %{{.*}} : (i32) -> i64
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
----------------
Just a [nit]


================
Comment at: flang/test/Fir/memref-data-flow.fir:142-143
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK-LABEL: ^bb2:
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK-NOT:     fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
----------------
To make sure that there's no `fir.store` before `fir.load`


================
Comment at: flang/test/Fir/memref-data-flow.fir:252
+
+// CHECK-LABL:  func forward_store2
+// CHECK-LABEL: ^bb2:
----------------



================
Comment at: flang/test/Fir/memref-data-flow.fir:257-260
+// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
----------------
Just a [nit]


================
Comment at: flang/test/Fir/memref-data-flow.fir:269-272
+// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
----------------



================
Comment at: flang/test/Fir/memref-data-flow.fir:400
+
+// CHECK-LABEL: func @forward_store3
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
----------------
To make sure that there's no `fir.store` before `fir.load`.


================
Comment at: flang/test/Fir/memref-data-flow.fir:426-429
+// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
----------------
[nit]


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111288/new/

https://reviews.llvm.org/D111288