[PATCH] D111555: [LoopVectorize] Add strict reduction support for fmuladd intrinsic

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9618
+            new VPInstruction(Instruction::FMul, FMulOps);
+        FMulRecipe->setUnderlyingInstr(R);
+        WidenRecipe->getParent()->insert(FMulRecipe,
----------------
fhahn wrote:
> RosieSumpter wrote:
> > fhahn wrote:
> > > david-arm wrote:
> > > > Hi @RosieSumpter, I realise you were asked to make this change, but it also doesn't feel right to be setting an underlying instruction here because there isn't one really. The underlying instruction is the fmuladd call and is already added to the VPReductionRecipe, so adding to two recipes feels a bit dangerous? Perhaps we should be adding a new interface to VPInstruction instead that allows setting the 'FastMathFlags' for the instruction?
> > > Agreed, we shouldn't set the underlying instruction explicitly here, visitbility is intentionally restricted. I don't think the FMF changes should be pulled into this change. Can the setting of FMFs be moved to a follow-up patch?
> > Hi @fhahn, thanks for the comment. I have instead added a ##setFastMathFlags## method to the ##VPInstruction## class. At the moment I've left it as part of this patch as, after discussion with @david-arm, it seems that it may not be ideal to split out this change given that this would mean submitting code that requires a fix. Also, it doesn't look like VPInstruction has been used for FP operations elsewhere, so currently this change is only used in the case of fmuladd being used. If you do still think it would be better as a follow-up patch though I'm happy to do that instead. 
> Could you elaborate what you mean by 'requires a fix'? The code would still be correct without FMFs, just be not as optimal as it could be, right? 
> 
> It's quite common to split out changes not directly related into separate patches as this makes it easier to review them individually rather than further extending an already big patch.
Hi @fhahn, @RosieSumpter has split out other patches before so I think she is aware why we sometimes split them up. It was actually my suggestion that we might want to keep this in the same patch. I think it depends upon your perspective about whether adding the flags is a "nice to have" or "just broken". Personally to me it feels like the latter because we're not propagating user-specified flags and getting the requested behaviour. If it has to be split out that's fine, but it does feel a bit counter-intuitive.


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

https://reviews.llvm.org/D111555