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

Thu Nov 4 11:42:43 PDT 2021

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))
----------------
schweitz wrote:
> clementval wrote:
> > awarzynski wrote:
> > > mehdi_amini wrote:
> > > > schweitz wrote:
> > > > > mehdi_amini wrote:
> > > > > > clementval wrote:
> > > > > > > mehdi_amini wrote:
> > > > > > > > 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
> > > > > > > > 
> > > > > > > We have talked together and we do not think the main LLVM guideline should be change for that. We will update couple of `auto`s in the upstreaming patch where it makes sense. 
> > > > > > > We will also put up for review a update on the Flang guidelines like you have for MLIR (https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide) where we can precise the auto usage. Would that work for you? 
> > > > > > > 
> > > > > > > I'm gonna update this patch and the other that is under review.
> > > > > > Note that the only place where MLIR deviates from LLVM really is the `camelBack` aspect (the other items are just things that LLVM does not define or aren't applicable in LLVM), and that wasn't intentional to deviate from LLVM: at the time there was a push in LLVM to adopt `camelBack` and we used it in anticipation. Had we known that LLVM would end-up not changing, we likely would have followed LLVM convention as-is.
> > > > > > 
> > > > > > Looking forward to the proposal for flang, I hope it won't go agains the LLVM style guide!
> > > > > Mehdi:
> > > > > 
> > > > > Would you help clarify direction here? Are you suggesting that you would be ok with revising the LLVM style just for the flang directories in question (just like MLIR did with respect to camelBack)? Or do you prefer that this be addressed for the entire llvm-project tree?
> > > > > 
> > > > Unless there is a really good reason to, I don't think the flang directory is supposed to diverge from LLVM.
> > > > I understand the status quo (which was discussed on llvm-dev@ before deciding on integrating flang I believe) to be that we'll apply the existing LLVM coding standard (I remember that C++17 was an exception for example) and usual practices.
> > > > 
> > > > So I see two possible ways if you want to have flang code not follow the LLVM practices:
> > > > - renegotiate the status quo (I would go back to llvm-dev@ for this, since this is where this was agree'd upon before).
> > > > - update the LLVM-wide coding standards.
> > > > 
> > > > In general I rather keep consistency in the project, unless the specificities of a particular project are making a difference (for example libcxx is very constrained by the standard, the compiler-rt have "embedded" constraints, MLIR python bindings need exception handling, etc.)
> > > > 
> > > I believe that this is the most recent discussion on the usage of `auto` on llvm-dev: https://lists.llvm.org/pipermail/llvm-dev/2018-November/127953.html. Based on that, an update to the LLVM-wide coding standards is likely to be a hard sell.
> > > 
> > > Perhaps the focus should be on "why" would Flang benefit from a bit more liberal approach here?
> > Once again, I don't think we do not follow the LLVM guidelines with `auto`. We have eased our stance here by updating `auto` where requested and we will do so future patches as well.
> > I don't think Eric is proposing a divergence from the guidelines at all but rather a clarification for Flang for a guideline that is subject to interpretation.
> This concerns a very limited, narrow scope: local variable declarations with initialization expressions. The phrase "already obvious" in the coding conventions is ambiguous, an invitation to disagreement.
> 
> The use of `auto` allows the compiler to perform type inferencing on variables, resulting in the following benefits:
> 
> - Extremely long and complicated type names are not used, which is less error-prone and greatly improves understandability.
> - The clutter from repetitive namespace tags is removed.
> - Explicitly declared types can and do introduce hard-to-find bugs from unexpected or incorrect truncations, extensions, and other conversions.
> - Using `auto` instead of declared types simplifies refactoring when interfaces are rapidly evolving.
> - `auto` can be required when writing some templatized code.
> 
Right, what you write here seems reasonable. But you have to see how it plays in practice, and the 2 answers in the thread above are a good reference.

Also, if we come back at that case at hand here:

```
  for (auto *user : v.getUsers())
```

vs 

```
  for (mlir::Operation *user : v.getUsers())
```

I don't think this is an "Extremely long and complicated type name" case, the namespace is annoying, what I'm doing in every project downstream (like TensorFlow) is to have a namespace inside the mlir namespace (`mlir::tensorflow::`). If your code is implemented inside this namespace, you remove all of the prefixes.

> Explicitly declared types can and do introduce hard-to-find bugs from unexpected or incorrect truncations, extensions, and other conversions.

It's be great to have a very clear guideline on the kind of API that is subject to these problems, any reference by the way?

> Using auto instead of declared types simplifies refactoring when interfaces are rapidly evolving.

Yes, but this is also a double-edged sword (code may compile when you wouldn't expect it possibly). Also, this isn't a rapidly evolving API here.

For this particular `getUsers()` API, it isn't defined in this file, so I wouldn't say that the context is obvious here to just know the type: having to lookup an API on another class isn't the current context to me.
And in this particular example it is worse because there is a very close API: `v.getUses()` (it differs by one letter!).
>From experience, this users/uses is also a frequent source of confusion for users.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111288