[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:581
                                    RTLIB::POWI_F128,
                                    RTLIB::POWI_PPCF128);
   if (!TLI.getLibcallName(LC)) {
----------------
bjope wrote:
> efriedma wrote:
> > bjope wrote:
> > > efriedma wrote:
> > > > 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.
> > > 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.
> > 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.
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99439/new/

https://reviews.llvm.org/D99439