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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp:29
+  std::vector<OpT> ops;
+  for (auto *user : v.getUsers())
+    if (auto op = dyn_cast<OpT>(user))
----------------
clementval wrote:
> mehdi_amini wrote:
> > mehdi_amini wrote:
> > > 
> > This is marked as done, but it isn't (it isn't the only one I think)
> I have discussed this with others and they would like to keep it like that. The guidlines are kind of blurry on this... If this is a blocker we will ave to discuss this further. 
Let's discuss then.
(and please don't ignore comments or mark them as done when they aren't!)


================
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>
----------------
awarzynski wrote:
> 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.
If I had to document your check right now it would be (as I understand them):

```
// Check that there is at least one fir.load followed by at least one fir.store, 
// both on i32. The load/store don't have to be related to each other.
```

And I could write the exact same test with:
```
// CHECK-LABEL: ^bb2:
// CHECK:     fir.load{{.*}} !fir.ref<i32>
// CHECK:     fir.store{{.*} }!fir.ref<i32>
```

Do you see an input to FileCheck that would be handled differently by the code you have right now and the above?

Also please provide the kind of plain text english documentation I wrote above on what you're checking in each section.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111288