Page MenuHomePhabricator

[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 Timeline

efriedma created this revision.Oct 15 2019, 5:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2019, 5:35 PM

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 added a comment.Oct 16 2019, 1:44 PM

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); }
efriedma updated this revision to Diff 225529.Oct 17 2019, 2:50 PM
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..
efriedma edited the summary of this revision. (Show Details)

Removed redundant check. Added support for x86.

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 marked an inline comment as done.Oct 18 2019, 11:58 AM
efriedma added inline comments.
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 added inline comments.Wed, Oct 30, 1:46 PM
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 this revision to Diff 227981.Tue, Nov 5, 4:30 PM

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 accepted this revision.Wed, Nov 6, 1:12 PM

lgtm

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.

This revision is now accepted and ready to land.Wed, Nov 6, 1:12 PM
This revision was automatically updated to reflect the committed changes.