[PATCH] D114460: [fir] Add fir reduction builder

Tue Nov 30 00:15:06 PST 2021

awarzynski added inline comments.

================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/Reduction.h:21
+
+/// Generate call to all runtime routine.
+/// This calls the descriptor based runtime call implementation of the all
----------------
clementval wrote:
> awarzynski wrote:
> > clementval wrote:
> > > awarzynski wrote:
> > > > clementval wrote:
> > > > > kiranchandramohan wrote:
> > > > > > awarzynski wrote:
> > > > > > > rovka wrote:
> > > > > > > > clementval wrote:
> > > > > > > > > awarzynski wrote:
> > > > > > > > > > rovka wrote:
> > > > > > > > > > > Should we use backticks or something around function names? It reads really strange otherwise.
> > > > > > > > > > > Also, isn't this actually AllDim?
> > > > > > > > > > > Should we use backticks or something around function names? 
> > > > > > > > > > +1
> > > > > > > > > > 
> > > > > > > > > > Also, what is the difference between `genAllDescriptor` and `genAll`? Why do we need both?
> > > > > > > > > The comments are pretty clear on the differences. 
> > > > > > > > But to clarify: the runtime sometimes has different entry points for the same intrinsic, for special cases. In this case, IIUC, there's All and AllDim, both of which implement the ALL intrinsic. If you look in the standard, you'll see there's 2 variants: ALL(MASK) or ALL(MASK, DIM).
> > > > > > > > (...) to clarify (...)
> > > > > > > 
> > > > > > > Many thanks @rovka !
> > > > > > > 
> > > > > > > >  If you look in the standard (...)
> > > > > > > Ah, I see! In fact, I think that it would make a lot of sense to refer to the standard here and to use the notation from there as well (like in Diana's comment).
> > > > > > > 
> > > > > > > > The comments are pretty clear on the differences.
> > > > > > > 
> > > > > > > I disagree. There is `genAllDescriptor` and `genAll`, but there's no reference to any "descriptor" in the comments, arguments or the implementation. Also, both methods seem to "Generate call to all runtime routine", but one returns an `mlir::Value`, whereas the other is `void`. If both basically generate a call, then why are the interfaces completely different? I appreciate the this implementation may depend on the APIs in the runtime, but it still does not explain what this "descriptor" in `genAllDescriptor` is and what are the return values are.
> > > > > > For the descriptor version, the result is in an array for which a descriptor is needed. It is probably created by the runtime.
> > > > > > I disagree. There is genAllDescriptor and genAll, but there's no reference to any "descriptor" in the comments, arguments or the implementation. Also, both methods seem to "Generate call to all runtime routine", but one returns an mlir::Value, whereas the other is void. If both basically generate a call, then why are the interfaces completely different? I appreciate the this implementation may depend on the APIs in the runtime, but it still does not explain what this "descriptor" in genAllDescriptor is and what are the return values are.
> > > > > There is a reference to descriptor in the comment and there is a `result` argument which is the "descriptor". 
> > > > > 
> > > > > 
> > > > Thank you all for clarifying. This is starting to make sense. I still believe that the comments for these hooks should be improved and clarify the underlying logic. Please see my suggestion at the top of this file. WDYT? 
> > > I'm not sure your comment represents what's happening. It describes what happens in the runtime but here we are just interested to generate the call and anything related to the runtime should be documented there. 
> > > I'm not sure your comment represents what's happening. 
> > 
> > Sure, it's just a suggestion and what I got from the conversation here. How could we improve it?
> > 
> > > It describes what happens in the runtime 
> > 
> > That wasn't the intention. However, as I understand, the implementation here is dictated by the implementation of the runtime. So one cannot really explain this API without discussing the runtime.
> > 
> > Please, feel free to discard my suggestion. However, I would really appreciate if you added some other additional documentation instead. Something that would hint the reasons behind `genAllDescriptor` and `genAll` having almost completely different signatures. That's the most visible difference here ATM. 
> Honestly, I'm not sure I follow you here. The signature are not *completely* different. One takes a box (descriptor) to hold the result and the other is a specialized version for rank one so no box and a return value. The comments on both functions are mentioning that. How it is handle in the runtime does not belong here IMO since we are just creating a call to the runtime.
> 
IMHO, the comments and naming used here are clear to somebody already familiar with this API and the wider context of how runtime calls are generated, but not so much to somebody new to this. Personally, I was missing that context when reading this file for the first time and the comments from @rovka and @kiranchandramohan really helped clarifying that. Thank you! Preserving their comments as extra documentation in this file felt like a natural and a low-cost thing to do that would be beneficial to the future Flang developers.

This is not a blocker for me.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114460