[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:
> clementval wrote:
> > clementval wrote:
> > > mehdi_amini wrote:
> > > > 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!)
> > > Sorry for that. We have discussed in this patch or in maybe another one and in the last call that during the upstreaming work any "blocker" comment should be put as "Change Requested" so that we can identified where to put our effort. 
> > > In this particular case `auto` can surely be removed but not everywhere like you mentioned in your comment. I think that's more the everywhere that others were against. 
> > > 
> > > > `auto` does not help readability so why not spell out the type? (here and everywhere, in all the upstreaming patches please).
> > On this particular comment, I remember what happened. I did the change and marked it as done then after discussion I revrted the change but I forgot to mention that. Sorry for that, I didn't mean to discard your comment. 
> @mehdi_amini We had discussion with the team and they mentioned couple of points why they would like to keep `auto`: 
> - It avoids to introduce type's bugs like
> `auto ch = getc(file);`
> changed to 
> `char ch = getc(file);`
> it's a small type change. The char type looks like documentation but it is actually introducing a bug that might be really hard to detect. 
> - Also refactoring is easier if we have type inference.
> Not sure if this is addressing your comment here. If not please let us know and we can discuss further how we can best approach this since it will be a recurrent pattern in the Flang code. 
> 
> 
These are very valid arguments, but they challenge the documented status quo in LLVM. As such I don't think this is something that can just be settled here in a revision.

I invite you to send an RFC (and a patch) to llvm-dev@ to update the coding standards if this is something that "the team" really believe is important to change: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable



================
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
----------------
awarzynski wrote:
> 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?
> Wouldn't that be sufficient?

Not really: this isn't really a good way to write and design tests IMO.

Ideally I'd like the tests to look like this for example: https://github.com/llvm/llvm-project/blob/main/mlir/test/Transforms/loop-coalescing.mlir
Note how every individual case is shared in a different function, and how documented they are in what they are actually testing.






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111288