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.)
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.)
I do believe that we need it for all the targets (x86, x86_64, armv7, aarch64).
lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
529 ↗ | (On Diff #225141) | Is there a reason to have the check do both? Is the latter not sufficient? The reason that we need to fall back to pow for powi is because MSVCRT does not support it, it has not much to do with the fact that it is Windows. That is - I can see this applying equally well to say ReactOS, which I suppose would be treated as Windows, but isn't really Windows. |
lib/Target/ARM/ARMISelLowering.cpp | ||
1163 ↗ | (On Diff #225141) | Similar |
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?
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp | ||
---|---|---|
581–582 ↗ | (On Diff #225141) | Can this be made into an assert or non-fatal error by DAG->getContext().emitError("..."), and then returning the input SDNode unmodified? |
I do believe that we need it for all the targets (x86, x86_64, armv7, aarch64).
Okay.
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.
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); }
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.
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp | ||
---|---|---|
581–582 ↗ | (On Diff #225141) | I would rather have this be a report_fatal_error or the DAG->getContext().emitError + return SDNode(). Please don't make this into an assertion, I know too many people who tend to use the -Asserts variant of the toolchain. |
lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp | ||
---|---|---|
581–582 ↗ | (On Diff #225141) | I think this is impossible to hit at the moment; there's no such thing as soft-float Windows. I just didn't want to make it an llvm_unreachable in case someone adds a new target where it might be relevant. Or I could implement it without a testcase, I guess; it's not hard. |
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.
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.