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

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

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.



================
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.)
----------------
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.


================
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)
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:2229
+  // appropriate FastMathFlags for reassociation enabled.
+  if (I->getType()->isFPOrFPVectorTy() && !hasFPAssociativeFlags(I))
     return;
----------------
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.


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

https://reviews.llvm.org/D129523