[PATCH] D129523: [Reassociate] Enable FP reassociation via 'reassoc' and 'nsz'

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

In D129523#3655223 <https://reviews.llvm.org/D129523#3655223>, @spatel wrote:

> This is a straightforward translation of the existing FMF checks, so LGTM. 
> I added some inline comments for potential cleanups that could be done as NFC commits either before or after.

Thanks for the review @spatel.  I'll update the comment and commit.  And I'll revisit the suggested cleanups a bit later.



================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:145-147
+/// Return true if I is an instruction with the FastMathFlags that are needed
+/// for general reassociation set.  (This routine is only intended to be called
+/// for floating-point operations.)
----------------
spatel wrote:
> It would be good to add a line to the code comment to state the non-obvious bit that came up in this review.
> Something like this:
>   /// This is not the same as testing Instruction::isAssociative() because it includes 
>   /// operations like fsub.
Good idea Sanjay.  I'll do that before committing.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:156
 static BinaryOperator *isReassociableOp(Value *V, unsigned Opcode) {
   auto *I = dyn_cast<Instruction>(V);
   if (I && I->hasOneUse() && I->getOpcode() == Opcode)
----------------
spatel wrote:
> Existing issue: this is awkward - we're dyn_casting to Instruction, but then we nakedly cast to BinaryOperator on the return. Could this cast to BO directly? There might be some subtlety with binop constant expressions that needs to be addressed.
Good point.  I'll be out of the office for a week, and I don't want to rush into this change (but I do want to get the main fix done now).  So I'll revisit this point when I return.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:2229
+  // appropriate FastMathFlags for reassociation enabled.
+  if (I->getType()->isFPOrFPVectorTy() && !hasFPAssociativeFlags(I))
     return;
----------------
spatel wrote:
> Existing issue: checking the type isn't consistent with the other diffs that check if the value is an FPMathOp. I'm not sure what, if any, functional difference it would make to the pass, but it seems wrong that these aren't using the same condition.
Another good point. Again, I'll revisit this point when I return.


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

https://reviews.llvm.org/D129523