[PATCH] D74708: [MLIR][Ploops] Add custom builders from ParallelOp and ReduceOp.

Wed Feb 19 08:33:07 PST 2020

mehdi_amini added inline comments.

================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:188
+              "ValueRange lowerBounds, ValueRange upperBounds, "
+              "ValueRange steps, ArrayRef<Type> resultTypes">
   ];
----------------
herhut wrote:
> pifon2a wrote:
> > mehdi_amini wrote:
> > > Isn't a loop always supposed to capture as operands the same types as the returned values? If so then why do we have a builder with `resultTypes`? I'd expect these to be inferred from the captured operands?
> > > 
> > > It is a problem with the current reduce, which does not model this, but that's a bug to fix right? And so adding a builder around the incorrect design does not seem right to me.
> > Ploops don't really capture any operands from which we can infer result types. The result types correspond to the ReduceOps in the body of the ploop.
> We have not added neutral values that can be used as initial values to parallel loops, yet, and I am not sure whether they should be mandatory. Having neutral values unlocks some additional reduction schemes but they are not always necessary. If we make them optional, we will need to provide return types to support that. 
> If we make them mandatory, we can adapt the builders and their use sites.
I thought we discussed this and it would be added?
There is no guarantee that a loop would be executed once: as such it should take an input value for all returned to support this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74708