[PATCH] D111085: [FPEnv][InstSimplify] Fold constrained X + -0.0 ==> X

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.

Thanks for the minimal patch!
LGTM - see inline for some cosmetics.



================
Comment at: llvm/include/llvm/IR/FPEnv.h:64
+/// at run time.
+inline bool RoundingModeMayBe(RoundingMode RM, RoundingMode QRM) {
+  return RM == QRM || RM == RoundingMode::Dynamic;
----------------
Nit: If we don't use the current/correct formatting for a function name (`roundingModeMayBe`), we're never going to get out of the formatting mess we're in. 

Bonus points for fixing all of the functions here (as an NFC preliminary patch). :)


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4949
 
+  // fadd X, -0 ==> X
+  if (canIgnoreSNaN(ExBehavior, FMF) &&
----------------
This is complicated logic, so it would be good to put a code comment/examples on it. Something like:
  // With strict/constrained FP, we have these possible edge cases that do not simplify to Op0:
  // fadd SNaN, -0.0 --> QNaN
  // fadd +0.0, -0.0 --> -0.0 (with round toward negative)


================
Comment at: llvm/test/Transforms/InstSimplify/strictfp-fadd.ll:69
 ;
   %ret = call float @llvm.experimental.constrained.fadd.f32(float %a, float -0.0, metadata !"round.downward", metadata !"fpexcept.ignore") #0
   ret float %ret
----------------
Independent of this patch, but I wonder if it is too late to unify the vocabulary on the metadata and enum names. 
Ie, why do we have 2 ways to spell "downward" / "TowardNegative", etc?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111085