This is an archive of the discontinued LLVM Phabricator instance.

Update @llvm.powi to handle different int sizes for the exponent
ClosedPublic

Authored by bjope on Mar 26 2021, 1:09 PM.

Details

Summary

This can be seen as a follow up to commit 0ee439b705e82a4fe20e2,
that changed the second argument of powidf2, powisf2 and
powitf2 in compiler-rt from si_int to int. That was to align with
how those runtimes are defined in libgcc.
One thing that seem to have been missing in that patch was to make
sure that the rest of LLVM also handle that the argument now depends
on the size of int (not using the si_int machine mode for 32-bit).
When using
builtin_powi for a target with 16-bit int clang crashed.
And when emitting libcalls to those rtlib functions, typically when
lowering @llvm.powi), the backend would always prepare the exponent
argument as an i32 which caused miscompiles when the rtlib was
compiled with 16-bit int.

The solution used here is to use an overloaded type for the second
argument in @llvm.powi. This way clang can use the "correct" type
when lowering __builtin_powi, and then later when emitting the libcall
it is assumed that the type used in @llvm.powi matches the rtlib
function.

One thing that needed some extra attention was that when vectorizing
calls several passes did not support that several arguments could
be overloaded in the intrinsics. This patch allows overload of a
scalar operand by adding hasVectorInstrinsicOverloadedScalarOpd, with
an entry for powi.

Diff Detail

Event Timeline

bjope created this revision.Mar 26 2021, 1:09 PM
bjope requested review of this revision.Mar 26 2021, 1:09 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 26 2021, 1:09 PM
bjope added a comment.Apr 26 2021, 7:35 AM

Ping!

(This patch is now 1 month old, has a number of reviewers and 41 subscribers, but not a single comment yet. I believe that if you aren't comfortable with reviewing, then it is perfectly OK to remove yourself as reveiwer to let the author know that the original set of reviewers was wrong. But this "silent treatment" is really painful.)

fhahn added a subscriber: fhahn.Apr 26 2021, 7:54 AM

Probably would be good to split this up in separate LLVM/Clang parts. Do we need to auto-upgrade calls to llvm.powi?

llvm/docs/LangRef.rst
13648

Not sure about the sentence about the size of the exponent. It refers to int and __powi, both of which are not really defined in the LangRef and it is not clear to what they are referring to here.

llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
576–577

Why just allow i32 and i16 here?

Could we restrict the supported offset types to i32 or smaller?

llvm/test/Transforms/InstCombine/pow_fp_int16.ll
1

I don't think we can rely on any specific triple here. If a triple is needed it should be in a sub-directory.

This comment was removed by xbolva00.
bjope added inline comments.Apr 26 2021, 8:37 AM
llvm/docs/LangRef.rst
13648

I see your point. Maybe something like "The type of the exponent should match the libm implementation for a target that lower the intrinsic to such a libcall.". Or maybe I should just skip trying to say something about it here.

llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
576–577

I think that i16 and i32 are the only sizes of "int" that are supported by any in-tree target. And people tend to get very upset if trying to generalize code outside the scope given by in-tree targets.

llvm/test/Transforms/InstCombine/pow-4.ll
4

Need to move these tests to a msp430 subdir.

llvm/test/Transforms/InstCombine/pow_fp_int16.ll
1

Yes, right, I need to move this.

bjope added inline comments.Apr 26 2021, 12:23 PM
llvm/test/Transforms/InstCombine/pow_fp_int16.ll
1

A bit surprised that there are several tests in test/Transform/InstCombine/ that use -mtriple without the test case being in a target specific subfolder. For example test/Transform/InstCombine/pow-1.ll is verifying 12 different mtriple variants (plus the default). Is that OK? How could one know if a target specific dir is needed when using -mtriple (unless doing some experiments with different targets-to-build settings)?

bjope added inline comments.Apr 26 2021, 12:45 PM
llvm/test/Transforms/InstCombine/pow-4.ll
4

Or maybe not. This actually works even with LLVM_TARGETS_TO_BUILD=Sparc, without being moved to a subdir (and the test ends up as PASSED rather than UNSUPPORTED). So this just follow how it is done for lots of other test cases that verify libcall support in InstCombine.

llvm/test/Transforms/InstCombine/pow_fp_int16.ll
1

This actually works even with LLVM_TARGETS_TO_BUILD=Sparc, without being moved to a subdir (and the test ends up as PASSED rather than UNSUPPORTED). So so this just follow how things are done for lots of other test cases that verify libcall support in InstCombine, and moving it to a subdir with a requirement to include MSP430 in the build only limits the amount of configs when the test case is executed.

xbolva00 added inline comments.May 11 2021, 1:34 PM
llvm/test/Transforms/InstCombine/pow_fp_int16.ll
3–5

Precommit? And we dont need full copy of existings tests - 2-3 tests for 16bit int are anough.

What about IR backward compatibility?

Seems reasonable. I'd like to see a test for autoupgrade; not sure if you need to make any code changes for that.

llvm/docs/LangRef.rst
13648

Maybe make this a bit more explicit: move the description of the exponent into a separate paragraph, and explicitly state "generally, the only exponent supported is the C type int".

llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
579–580

This is missing a diagnostic for the exponent. We don't want to silently miscompile if someone uses an exponent that isn't supported by the target.

bjope added inline comments.May 13 2021, 1:59 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
579–580

Not sure exactly what you suggest. Is that a general comment for all places in SelectionDAG where we may emit calls to RTLIB::POWI or what makes this SoftenFloatRes special?

If we end up using mismatching types in the call, wouldn't that being detected as ICE elsewhere? Only reason I made changes to this function in the first place was due to the historical assert above regarding the type of the exponent in FPOWI. Maybe I should just drop that assert instead? This is the only place where that is checked, but I figure that the SoftenFloatRes legalization is just one out of many places where FPOWI is legalized and lowered into libcalls to RTLIB::POWI.

efriedma added inline comments.May 13 2021, 12:06 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
579–580

It's a general issue with emitting calls to RTLIB::POWI; the second parameter to the call has to have type "int", to match the definition in libgcc/compiler-rt. I guess there are a few other places that also emit calls to these functions.

If we end up using mismatching types in the call, wouldn't that being detected as ICE elsewhere?

In SelectionDAG, function/pointer types don't exist; the callee of a function call is just a integer. So we'd never detect mismatched types; we'd just silently emit a call using the wrong calling convention.

bjope added inline comments.May 17 2021, 5:01 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
579–580

One interesting thing when trying to add checks verifying that DAG.getLibInfo().getIntSize() == Node->getOperand(1 + Offset).getValueType().getSizeInBits()) in LegalizeDAG some RISCV (64-bit) test cases fail. Looks like type legalization is promoting the exponent by replacing

    t5: i64,ch = CopyFromReg t0, Register:i64 %1
  t6: i32 = truncate t5
t7: f32 = fpowi t3, t6

by

    t5: i64,ch = CopyFromReg t0, Register:i64 %1
  t13: i64 = sign_extend_inreg t5, ValueType:ch:i32
t7: f32 = fpowi t3, t13

I kind of suspect that promoting the exponent for FPOWI always would be incorrect, if the idea is that the type always should match with sizeof(int).

In this case RISCV would lower the fpowi to a libcall like this

    t5: i64,ch = CopyFromReg t0, Register:i64 %1
  t13: i64 = sign_extend_inreg t5, ValueType:ch:i32
t20: ch,glue = CopyToReg t18, Register:i64 $x11, t13, t18:1
t23: ch,glue = RISCVISD::CALL t20, TargetExternalSymbol:i64'__powisf2' [TF=2], Register:i64 $x10, Register:i64 $x11, RegisterMask:Untyped, t20:1

using a 64-bit argument for the call, while the callee expects a 32-bit int. Depending on the calling conventions for RISCV64 I suppose this might work by coincidence, or it is a bad miscompile.

Not sure exactly how to deal with that when considering this patch. I was kind of aiming at fixing problems for 16-bit targets. Maybe we need to deal with DAGTypeLegalizer::PromoteIntOp_FPOWI first, turning it into a fault situation. And to do that one need to handle FPOWI for RISCV in some sort of way to make the 32-bit exponent legal first?

efriedma added inline comments.May 17 2021, 11:02 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
579–580

We probably end up getting lucky due to the RISCV calling convention... but it's ugly.

I think the right solution here is to force type legalization to generate the call (when we try to legalize the integer operand), instead of waiting for LegalizeDAG. That should allow the call lowering code to use the right calling convention.

bjope updated this revision to Diff 347015.May 21 2021, 6:36 AM

Addressed review comments (and rebased). This includes:

  • Add diagnostics for wrong sized exponent when lowering FPOWI to libcall.
  • For the above to work this patch now depends on D102918 to avoid that RISCV (64-bit) is promoting the exponent in FPOWI in a way that makes the operation undef.
  • Patch now also depends on pre-commit of a test case in D102919.
  • Updated the langref part about llvm.powi.
  • Updated the ISelOpcodes description about undefined exponent types in FPOWI.
  • Added auto-upgrade tests for llvm.powi.
bjope marked 3 inline comments as done.May 21 2021, 6:43 AM
bjope added inline comments.
llvm/test/Transforms/InstCombine/pow_fp_int16.ll
3–5

I've pre-commited the tests now. Remove some of the tests (mainly the ones related to i64. But I think most of the others are relevant as regression test to see that we get what is expected for the different scenarios also with 16-bit int.

bjope updated this revision to Diff 347513.May 24 2021, 2:49 PM

Rebased. This now depends on D102950 rather than D102918.

This revision is now accepted and ready to land.Jun 16 2021, 4:35 PM
This revision was landed with ongoing or failed builds.Jun 17 2021, 12:39 AM
This revision was automatically updated to reflect the committed changes.

LGTM

Thanks!

Just to mention, 'llvm.experimental.constrained.powi' uses i32. Probably not a big deal, just small inconsistency with llvm.powi.