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

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 3 16:09:30 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:
> 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.)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111288



More information about the llvm-commits mailing list