Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
llvm/include/llvm/IR/FPEnv.h | ||
---|---|---|
64 | I didn't notice that quirk of naming... |
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? |
llvm/include/llvm/IR/FPEnv.h | ||
---|---|---|
64 | I am OK with it. |
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). :)