[AArch64][X86] Don't assume __powidf2 is available on Windows.

[efriedma]

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

[compnerd]

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

[rnk]

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?

[efriedma]

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); }

[efriedma]

Removed redundant check. Added support for x86.

[compnerd]

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.

[efriedma]

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.

[compnerd]

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

581–582 ↗

(On Diff #225141)

@efriedma yeah, I think that both of us are in agreement, I just meant that leave this as is, or use the reporting error as @rnk suggested, but don't make it an assert is all.

[efriedma]

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.

[rnk]

lgtm

In D69013#1734847, @efriedma wrote:

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.

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.