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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/test/Fir/memref-data-flow.fir:62
+// CHECK-NOT:     fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK-LABEL: ^bb2:
----------------
mehdi_amini wrote:
> The two CHECK-NOT above apply to bb1, but in the source it is:
> 
> ```
> ^bb1(%2: index, %3: index):  // 2 preds: ^bb0, ^bb2
>   %4 = arith.cmpi sgt, %3, %c0 : index
>   cond_br %4, ^bb2, ^bb3
> ```
> 
> Why are we specifically checking for no load/store in here?
Currently, `fir-memref-dataflow-opt` makes sure that:
> // All store-load chains are removed

These particular `CHECK-NOT`s verify that there are no new store-loads in `bb1`. There shouldn't be. It's just a sanity check in case something goes horribly wrong. Do you think that it's confusing?


================
Comment at: flang/test/Fir/memref-data-flow.fir:65-66
+// CHECK-NOT:     fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+// CHECK-NOT:     %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+// CHECK:         fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
----------------
mehdi_amini wrote:
> I really don't understand this sequence: we have a CHECK-NOT followed by the identical CHECK.
IIUC, the first `CHECK-NOT` can safely be removed without affecting the test. From https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-not-directive :
> The “CHECK-NOT:” directive is used to verify that a string doesn’t occur between two matches (or before the first match, or after the last match). 
I also find it confusing, but I believe that it's valid and that it does check that there's no `store-load` chain here.


================
Comment at: flang/test/Fir/memref-data-flow.fir:276
+
+func @forward_store3(%arg0: !fir.ref<!fir.array<?x?xi32>>, %arg1: !fir.ref<!fir.array<?x?xi32>>, %arg2: !fir.ref<!fir.array<?x?xi32>>, %arg3: !fir.ref<i32>, %arg4: !fir.ref<i32>) {
+  %c100_i32 = arith.constant 100 : i32
----------------
mehdi_amini wrote:
> I still believe we should have documentation for the test itself: what is this IR exposing as a pattern and what it this testing that the other above don't?
> 
> This is quite a long function right now...
> what it this testing that the other above don't

IIUC, it is similar to the other tests. I don't think there's anything particular here beyond: there are some store-load chains, which are being removed. Wouldn't that be sufficient?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111288