Page MenuHomePhabricator

[FPEnv][InstSimplify] Fold constrained X + -0.0 ==> X
ClosedPublic

Authored by kpn on Oct 4 2021, 11:50 AM.

Details

Summary

Currently the fadd optimizations in InstSimplify don't know how to do this "X + -0.0 ==> X" fold when using the constrained intrinsics. This adds the support.

This review is derived from D106362 with some improvements from D107285. I was asked to make a new, smaller version of D106362.

Diff Detail

Event Timeline

kpn requested review of this revision.Oct 4 2021, 11:50 AM
kpn created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2021, 11:50 AM
sepavloff accepted this revision.Oct 4 2021, 10:49 PM

LGTM.

llvm/lib/Analysis/InstructionSimplify.cpp
4930

Why not moving this function into header file in this patch? May be to FPEnv.h. You need this function in D107285.

This revision is now accepted and ready to land.Oct 4 2021, 10:49 PM
spatel accepted this revision.Oct 5 2021, 6:54 AM

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

llvm/include/llvm/IR/FPEnv.h
64

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

llvm/lib/Analysis/InstructionSimplify.cpp
4949

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)
llvm/test/Transforms/InstSimplify/strictfp-fadd.ll
69

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?

kpn added inline comments.Oct 6 2021, 8:13 AM
llvm/include/llvm/IR/FPEnv.h
64

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

llvm/lib/Analysis/InstructionSimplify.cpp
4930

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.

4949

Can do!

llvm/test/Transforms/InstSimplify/strictfp-fadd.ll
69

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.

spatel added inline comments.Oct 6 2021, 8:49 AM
llvm/include/llvm/IR/FPEnv.h
64

I didn't notice that quirk of naming...
In this case, I think we can fix it with "canRoundingModeBe" ?
That may lower our score on an English test, but still in compliance with:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

kpn added inline comments.Oct 6 2021, 8:58 AM
llvm/include/llvm/IR/FPEnv.h
64

That works!

Oh, @sepavloff, are you good with me adding, in a different commit, the word "convert" to the beginning of these four conversion functions?

sepavloff added inline comments.Oct 6 2021, 10:16 AM
llvm/include/llvm/IR/FPEnv.h
64

I am OK with it.

This revision was automatically updated to reflect the committed changes.