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

[Kevin P. Neal via Phabricator via llvm-commits]

kpn added inline comments.


================
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;
----------------
spatel wrote:
> 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). :)
Does this apply to type names that appear in the name of functions? For example, RoundingModeToStr() takes a RoundingMode argument. That was part of why I went with RoundingModeMayBe(). (The rest of the reason was to be consistent with the rest of this file.)


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4930
 
+// TODO: Move this out to a header file:
+static inline bool canIgnoreSNaN(fp::ExceptionBehavior EB, FastMathFlags FMF) {
----------------
sepavloff wrote:
> Why not moving this function into header file in this patch? May be to `FPEnv.h`. You need this function in D107285.
Yeah, I looked into that. I agree that FPEnv.h is the right place.

But when I tried it I found that FastMathFlags is in Operator.h, and that seems like a heavy file to include in FPEnv.h. In the near future I'm planning on trying to move the FastMathFlags out into a standalone header that I can pull into Operator.h and FPEnv.h, and that will allow me to move canIgnoreSNaN() into FPEnv.h. 


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4949
 
+  // fadd X, -0 ==> X
+  if (canIgnoreSNaN(ExBehavior, FMF) &&
----------------
spatel wrote:
> 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)
Can do!


================
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
----------------
spatel wrote:
> 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?
Good question. I see that llvm::RoundingMode mirrors the names in IEEE-754. So if we change anything it should be the strings accepted by the constrained intrinsics. I guess that's something someone could do especially since the constrained intrinsics are still in the experimental namespace? With upgrade support of course.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111085