We had some code for this for 32-bit ARM, but this doesn't really need to be in target-specific code; generalize it.
(I think this started showing up recently because we added an optimization that converts pow to powi.)
Paths
| Differential D69013
[AArch64][X86] Don't assume __powidf2 is available on Windows. ClosedPublic Authored by efriedma on Oct 15 2019, 5:35 PM.
Details Summary We had some code for this for 32-bit ARM, but this doesn't really need to be in target-specific code; generalize it. (I think this started showing up recently because we added an optimization that converts pow to powi.)
Diff Detail
Event TimelineComment Actions I do believe that we need it for all the targets (x86, x86_64, armv7, aarch64).
Comment Actions I tried to get clang to make llvm.powi from C, but it was challenging. Setting -fno-math-errno was what ended up mattering, and even then, I couldn't go from llvm.pow.f64 to powi. Then, I used powi directly from LLVM IR, and I couldn't get it to generate a libcall for x86. It just does repeated multiplication. So, I guess we don't need to do anything for x86?
Comment Actions
Okay.
We generally don't generate the libcall when the power is constant, only when it's a variable. To make clang form powi from pow, I think you need -ffast-math. Try something like the following: double pow(double,double); double f(double x, int y) { return pow(x,y); } efriedma retitled this revision from [AArch64] Don't assume __powidf2 is available on Windows. to [AArch64][X86] Don't assume __powidf2 is available on Windows.. Comment ActionsRemoved redundant check. Added support for x86. Comment Actions I do like @rnk's suggestion for the emission of the error, but Im not really tied to it - if it triggers, the TLI for the target is not setup properly or is missing the symbol, in which case, it needs to be addressed anyways. Beyond that, assuming that @rnk is okay with this, LGTM. Thanks for fixing this.
efriedma added inline comments.
Comment Actions Updated. I'm a little uncomfortable with the untested codepath, but I guess it isn't any worse than report_fatal_error, which always kills the process. Comment Actions lgtm
These days I'm trying to take a hard look at report_fatal_error and ask why it's not either unreachable, assert, or error. This revision is now accepted and ready to land.Nov 6 2019, 1:12 PM Closed by commit rG5df3a87224ef: [AArch64][X86] Don't assume __powidf2 is available on Windows. (authored by efriedma). · Explain WhyNov 8 2019, 12:43 PM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 225529 lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
lib/Target/AArch64/AArch64ISelLowering.cpp
lib/Target/ARM/ARMISelLowering.cpp
lib/Target/X86/X86ISelLowering.cpp
test/CodeGen/AArch64/powi-windows.ll
test/CodeGen/X86/powi-windows.ll
|
Can this be made into an assert or non-fatal error by DAG->getContext().emitError("..."), and then returning the input SDNode unmodified?