[RISCV] Avoid undef result due to promotion of FPOWI exponent

[bjope]

Summary

The 64-bit RISCV target has no legal i32 type. So when SelectionDAG
is legalizing the types for FPOWI the exponent is promoted to i64.
However, the FPOWI/STRICT_FPOWI are defined as having undefined
result if the exponent is larger than i32 (it should probably say
"if the exponent is larger than sizeof(int) for the target" as it
shold be possible to lower those nodes to RTLIB::POWI libcalls and
those runtime library functions is using "int" as input arguments).

This patch can be seen as a workaround to avoid that we promote
the exponent to i64 (making the result undefined). As a simple
solution we expand FPOWI/STRICT_FPOWI into SINT_TO_FP+FPOW. This
is the same rewrite as normally done by LegalizeDAG when the
RTLIB::POWI libcalls aren't allowed.

I leave it as a future FIXME to implement legalization/lowering
of FPOWI/STRICT_FPOWI into using RTLIB::POWI libcalls again. Someone
that knows about RISCV and calling conventions can perhaps figure
out how to do that properly.

[craig.topper]

Where is it documented that larger than i32 is undefined? I see this in ISDOpcodes.def

For FPOWI, the result is undefined if if the integer operand doesn't fit into 32 bits.

That sounds more like it’s talking about the range of the value than the type. So why isn’t it ok to sign extend i32 to i64?

[craig.topper]

Ok I missed the other patches this is linked to. The RISCV64 ABI is to pass int as an i64 that has been sign extended which is what we were doing.

So should we Custom type legalize to the exact sequence we were using before?

[bjope]

In D102918#2773820, @craig.topper wrote:

Where is it documented that larger than i32 is undefined? I see this in ISDOpcodes.def

For FPOWI, the result is undefined if if the integer operand doesn't fit into 32 bits.

That sounds more like it’s talking about the range of the value than the type. So why isn’t it ok to sign extend i32 to i64?

Right, one could argue that the value fits if doing a sign-extend. Then this patch might be a bit harsh as it regress the RISCV lowering without actually having depended on that noone folded the FPOWI to undef (as that only would be allowed if the value potentially could be out-of-range).

[bjope]

In D102918#2773859, @craig.topper wrote:

Ok I missed the other patches this is linked to. The RISCV64 ABI is to pass int as an i64 that has been sign extended which is what we were doing.

So should we Custom type legalize to the exact sequence we were using before?

Since it is a bit fragile to allow the generic promotion to promote the exponenet to a type larger than "sizeof(int)" I was looking for some way to handle this special case for RISCV64 (as that is the only target I've found that uses the generic promotion to implement the calling convention ABI sign-extend).

Before I uploaded this patch I made some attempts trying to implement this using some other hack. But I didn't feel confident with those hacks. Maybe I was hooking into the wrong place, but I guess one need to consider the soft floats (or not releveant for RISCV?) and strict fp (or not?) . And one still need to legalize the fp operand? And the result? So one would need to do all that using some custom lowering, right?

[craig.topper]

In D102918#2773859, @craig.topper wrote:

Ok I missed the other patches this is linked to. The RISCV64 ABI is to pass int as an i64 that has been sign extended which is what we were doing.

So should we Custom type legalize to the exact sequence we were using before?

In D102918#2773961, @bjope wrote:

In D102918#2773859, @craig.topper wrote:

Ok I missed the other patches this is linked to. The RISCV64 ABI is to pass int as an i64 that has been sign extended which is what we were doing.

So should we Custom type legalize to the exact sequence we were using before?

Since it is a bit fragile to allow the generic promotion to promote the exponenet to a type larger than "sizeof(int)" I was looking for some way to handle this special case for RISCV64 (as that is the only target I've found that uses the generic promotion to implement the calling convention ABI sign-extend).

It's quite likely the sign extend was added to LegalizeIntegerTypes specifically for RISCV.

Before I uploaded this patch I made some attempts trying to implement this using some other hack. But I didn't feel confident with those hacks. Maybe I was hooking into the wrong place, but I guess one need to consider the soft floats (or not releveant for RISCV?) and strict fp (or not?) . And one still need to legalize the fp operand? And the result? So one would need to do all that using some custom lowering, right?

RISCV does have soft float. SoftenFloatRes_FPOWI probably ends up promoting the type when it goes through makeLibCall. So it be ok already?

[bjope]

In D102918#2774007, @craig.topper wrote:

In D102918#2773859, @craig.topper wrote:

Ok I missed the other patches this is linked to. The RISCV64 ABI is to pass int as an i64 that has been sign extended which is what we were doing.

So should we Custom type legalize to the exact sequence we were using before?

In D102918#2773961, @bjope wrote:

In D102918#2773859, @craig.topper wrote:

Ok I missed the other patches this is linked to. The RISCV64 ABI is to pass int as an i64 that has been sign extended which is what we were doing.

So should we Custom type legalize to the exact sequence we were using before?

Since it is a bit fragile to allow the generic promotion to promote the exponenet to a type larger than "sizeof(int)" I was looking for some way to handle this special case for RISCV64 (as that is the only target I've found that uses the generic promotion to implement the calling convention ABI sign-extend).

It's quite likely the sign extend was added to LegalizeIntegerTypes specifically for RISCV.

Before I uploaded this patch I made some attempts trying to implement this using some other hack. But I didn't feel confident with those hacks. Maybe I was hooking into the wrong place, but I guess one need to consider the soft floats (or not releveant for RISCV?) and strict fp (or not?) . And one still need to legalize the fp operand? And the result? So one would need to do all that using some custom lowering, right?

RISCV does have soft float. SoftenFloatRes_FPOWI probably ends up promoting the type when it goes through makeLibCall. So it be ok already?

The problem this patch is intended to solve is that without it one would end up failing on the assert in DAGTypeLegalizer::PromoteIntOp_FPOWI that I planned to add in D99439.
So one would need to make sure we do something before type legalization is promoting the non-legal i32 operand in the FPOWI.
Btw, if just skipping the assert for RISCV one would later end up in the new diagnostic being added in LegalizeDAG in D99439. So I think one need to custom lower to a libcall quite early for RISCV if we want the diagnostics related to the exponent type.

Another idea is to skip adding those checks (risking miscompiles, relying on that targets are implementing regression tests that validate that the lowering into libcalls works as expected). I basically added those diagnostics based on a review comment from @efriedma. But I guess we risk using the wrong type for any libcall, so I'm not sure adding diagnostics specifically for FPOWI is doing much difference generally?

[efriedma]

I don't think this is the right approach... in particular, messing with target-specific code means that every target that doesn't have 32-bit registers would need to explicitly handle this.

What we should be doing is changing DAGTypeLegalizer::PromoteIntOp_FPOWI so it doesn't generate an FPOWI with a 64-bit operand, so we don't have an issue in the first place. We can generate the powi libcall from there, I think.

[bjope]

In D102918#2774155, @efriedma wrote:

I don't think this is the right approach... in particular, messing with target-specific code means that every target that doesn't have 32-bit registers would need to explicitly handle this.

What we should be doing is changing DAGTypeLegalizer::PromoteIntOp_FPOWI so it doesn't generate an FPOWI with a 64-bit operand, so we don't have an issue in the first place. We can generate the powi libcall from there, I think.

Yes. After digging around a bit I think I got something that hopefully looks ok. Took me awhile to understand that I shouldn't need to care about legalization of the result and fp operand (because those should have been legalized already when we get to type legalizing the exponent operand). And I also realized that I can provide the original (not promoted) operand to TargetLowering::makeLibCall and then lowerCallTo will handle the situation according to the calling convention (such as promoting using sign-extend for 64-bit RISCV).

So I created D102950 that hopefully can replace this patch by creating the libcall already in DAGTypeLegalizer::PromoteIntOp_FPOWI.

[bjope]

Abandoned in favor of https://reviews.llvm.org/D102950

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

 //===-- RISCVISelLowering.cpp - RISCV DAG Lowering Implementation  --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
RISCVTargetLowering::RISCVTargetLowering(const TargetMachine &TM,
   setTargetDAGCombine(ISD::AND);
   setTargetDAGCombine(ISD::OR);
   setTargetDAGCombine(ISD::XOR);
   if (Subtarget.hasStdExtV()) {
     setTargetDAGCombine(ISD::FCOPYSIGN);
     setTargetDAGCombine(ISD::MGATHER);
     setTargetDAGCombine(ISD::MSCATTER);
   }
+  if (Subtarget.is64Bit()) {
+    setTargetDAGCombine(ISD::FPOWI);
+    setTargetDAGCombine(ISD::STRICT_FPOWI);
+  }
 }
 
 EVT RISCVTargetLowering::getSetCCResultType(const DataLayout &DL,
                                             LLVMContext &Context,
                                             EVT VT) const {
   if (!VT.isVector())
     return getPointerTy(DL);
   if (Subtarget.hasStdExtV() &&
case ISD::MSCATTER: {
     }
     const auto *MSN = cast<MaskedScatterSDNode>(N);
     return DAG.getMaskedScatter(
         N->getVTList(), MGSN->getMemoryVT(), DL,
         {MGSN->getChain(), MSN->getValue(), MGSN->getMask(), MGSN->getBasePtr(),
          Index, MGSN->getScale()},
         MGSN->getMemOperand(), NewIndexTy, MSN->isTruncatingStore());
   }
+  case ISD::FPOWI:
+  case ISD::STRICT_FPOWI: {
+    if (!DCI.isBeforeLegalize())
+      break;
+    // FIXME: It should be possible to lower these to RTLIB::POWI libcalls. Now
+    // we instead rewrite into using FPOW in case the type of the exponent isn't
+    // legal (to avoid that TypeLegalization promotes the exponent in FPOWI to
+    // i64, as FPOWI has undefined result if the exponent is larger than i32).
+    if (Subtarget.is64Bit()) {
+      SDValue Exponent = DAG.getNode(ISD::SINT_TO_FP, SDLoc(N),
+                                     N->getValueType(0),
+                                     N->getOperand(1));
+      return DAG.getNode(ISD::FPOW, SDLoc(N),
+                         N->getValueType(0), N->getOperand(0), Exponent);
+    }
+    break;
+  }
   }
 
   return SDValue();
 }
 
 bool RISCVTargetLowering::isDesirableToCommuteWithShift(
     const SDNode *N, CombineLevel Level) const {
   // The following folds are only desirable if `(OP _, c1 << c2)` can be

llvm/test/CodeGen/RISCV/double-intrinsics.ll

 ; RV32IFD-NEXT:    lw ra, 12(sp) # 4-byte Folded Reload
 ; RV32IFD-NEXT:    addi sp, sp, 16
 ; RV32IFD-NEXT:    ret
 ;
 ; RV64IFD-LABEL: powi_f64:
 ; RV64IFD:       # %bb.0:
 ; RV64IFD-NEXT:    addi sp, sp, -16
 ; RV64IFD-NEXT:    sd ra, 8(sp) # 8-byte Folded Spill
-; RV64IFD-NEXT:    sext.w a1, a1
-; RV64IFD-NEXT:    call __powidf2@plt
+; RV64IFD-NEXT:    fcvt.d.w ft0, a1
+; RV64IFD-NEXT:    fmv.x.d a1, ft0
+; RV64IFD-NEXT:    call pow@plt
 ; RV64IFD-NEXT:    ld ra, 8(sp) # 8-byte Folded Reload
 ; RV64IFD-NEXT:    addi sp, sp, 16
 ; RV64IFD-NEXT:    ret
   %1 = call double @llvm.powi.f64(double %a, i32 %b)
   ret double %1
 }
 
 declare double @llvm.sin.f64(double)

llvm/test/CodeGen/RISCV/float-intrinsics.ll

 ; RV32IF-NEXT:    lw ra, 12(sp) # 4-byte Folded Reload
 ; RV32IF-NEXT:    addi sp, sp, 16
 ; RV32IF-NEXT:    ret
 ;
 ; RV64IF-LABEL: powi_f32:
 ; RV64IF:       # %bb.0:
 ; RV64IF-NEXT:    addi sp, sp, -16
 ; RV64IF-NEXT:    sd ra, 8(sp) # 8-byte Folded Spill
-; RV64IF-NEXT:    sext.w a1, a1
-; RV64IF-NEXT:    call __powisf2@plt
+; RV64IF-NEXT:    fcvt.s.w ft0, a1
+; RV64IF-NEXT:    fmv.x.w a1, ft0
+; RV64IF-NEXT:    call powf@plt
 ; RV64IF-NEXT:    ld ra, 8(sp) # 8-byte Folded Reload
 ; RV64IF-NEXT:    addi sp, sp, 16
 ; RV64IF-NEXT:    ret
   %1 = call float @llvm.powi.f32(float %a, i32 %b)
   ret float %1
 }
 
 declare float @llvm.sin.f32(float)

llvm/test/CodeGen/RISCV/rv64i-single-softfloat.ll

 
 declare float @llvm.powi.f32(float %Val, i32 %power)
 
 define float @fpowi_s(float %a, i32 %b) nounwind {
 ; RV64I-LABEL: fpowi_s:
 ; RV64I:       # %bb.0:
 ; RV64I-NEXT:    addi sp, sp, -16
 ; RV64I-NEXT:    sd ra, 8(sp) # 8-byte Folded Spill
-; RV64I-NEXT:    sext.w a1, a1
-; RV64I-NEXT:    call __powisf2@plt
+; RV64I-NEXT:    sd s0, 0(sp) # 8-byte Folded Spill
+; RV64I-NEXT:    mv s0, a0
+; RV64I-NEXT:    sext.w a0, a1
+; RV64I-NEXT:    call __floatsisf@plt
+; RV64I-NEXT:    mv a1, a0
+; RV64I-NEXT:    mv a0, s0
+; RV64I-NEXT:    call powf@plt
+; RV64I-NEXT:    ld s0, 0(sp) # 8-byte Folded Reload
 ; RV64I-NEXT:    ld ra, 8(sp) # 8-byte Folded Reload
 ; RV64I-NEXT:    addi sp, sp, 16
 ; RV64I-NEXT:    ret
   %1 = call float @llvm.powi.f32(float %a, i32 %b)
   ret float %1
 }
 
 define double @fp_ext(float %a) nounwind {