[llvm-dev] [SelectionDAG] Wrong type for shift libcalls when expanding funnelled shifts

[Björn Pettersson A via llvm-dev]

Could it perhaps be two bugs here?

After legalization the shift amount type should match TLI.getShiftAmountTy()
for SHL/SRL/ROT etc.

So if DAGTypeLegalizer::ExpandIntRes_Rotate is emitting SHL/SRL nodes that
has a different type for the shift amount that sounds a bit weird to me.
Or is that supposed to be legalized after having expanded the rotate result?


Nevertheless, DAGTypeLegalizer::ExpandIntRes_Shift can't assume that
the shift amount operand already has a been legalized, so it must
handle that the type isn't matching TLI.getShiftAmountTy().
Although, I don't know if TLI.getShiftAmountTy() necessarily need to
match with "si_int" that the LIBC function is expecting. For the libcall
to be correct (ABI wise), then I think the shift amount should be converted
to something that matches with DAG.getLibInfo().getIntSize().

/Björn

> -----Original Message-----
> From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Itay
> Bookstein via llvm-dev
> Sent: den 26 september 2021 21:21
> To: llvm-dev <llvm-dev at lists.llvm.org>
> Subject: [llvm-dev] [SelectionDAG] Wrong type for shift libcalls when
> expanding funnelled shifts
> 
> Hey all,
> 
> I've recently encountered a problem after upgrading the LLVM version
> of our downstream,
> and it seems to reproduce in other backends as well. It's a bit
> esoteric, as it's rare for
> shift libcalls to be emitted at all, but I've reproduced it on top of
> latest trunk and can
> demonstrate the subtle codegen issue on e.g. RISCV32 (using minsize to
> induce libcall
> instead of expand):
> 
> // fshr.ll
> declare i64 @llvm.fshr.i64(i64, i64, i64)
> 
> define i64 @foo(i64 %__val, i64 %__shift) minsize {
>   %cond = tail call i64 @llvm.fshr.i64(i64 %__val, i64 %__val, i64
> %__shift)
>   ret i64 %cond
> }
> 
> RUN:
> llc --mtriple=riscv32 fshr.ll -o -
> 
> OUTPUT:
> [snipped]
> mv s0, a2
> mv s2, a1
> mv s1, a0
> andi a2, a2, 63
> mv a3, zero ; This is redundant
> call __lshrdi3 at plt
> mv s3, a0
> mv s4, a1
> neg a0, s0
> andi a2, a0, 63
> mv a0, s1
> mv a1, s2
> mv a3, zero ; This is redundant
> call __ashldi3 at plt
> or a0, s3, a0
> or a1, s4, a1
> [snipped]
> 
> The problem is that TargetLowering::makeLibCall is called with
> Ops[1].getValueType() == MVT::i64
> from DAGTypeLegalizer::ExpandIntRes_Shift, which is incorrect because
> shift libcalls have MVT::i32
> for their second argument (. In this particular case, this leads to a
> harmless-but-redundant zeroing
> of a register, but in the general case it's an ABI mismatch.
> 
> The path that the SelectionDAG node undergoes is:
> 1. SelectionDAGBuilder::visitIntrinsicCall, Intrinsic::fshr, X == Y =>
> create ISD::ROTR.
> 2. DAGTypeLegalizer::ExpandIntRes_Rotate
> 3. TargetLowering::expandROT => No support for rotate, create shifts
> (but with ShVT = Op1.getValueType(), so MVT::i64)
> 4. DAGTypeLegalizer::ExpandIntRes_Shift => Libcallize => called with {
> N->getOperand(0), N->getOperand(1) } as operands, but the second
> operand is MVT::i64.
> 
> When I traced the path that regular shifts undergo, they start out
> life already with shift amount type according to TLI
> getShiftAmountType() + some logic to extend/truncate, see
> SelectionDAGBuilder::visitShift.
> 
> I think a correct fix would be to change ExpandIntRes_Shift's
> libcallization flow to ZExtOrTrunc (or potentially AnyExtOrTrunc) the
> second argument to MVT::i32,
> but I don't have enough perspective to be confident that that's a
> correct and sufficient fix.
> 
> Appreciate your input!
> --
> Itay Bookstein
> Software Engineer
> NEXTSILICON
> 
> --
> This e-mail message and any attachments thereto are intended only for the
> person or entity to which it is addressed and may contain confidential
> and/or privileged material. Any retransmission, dissemination, copying or
> other use of, or taking of any action in reliance upon this information is
> prohibited. If you are not the intended addressee, please contact the
> sender immediately and delete the materials and information from your
> device and system and confirm the deletion by reply e-mail.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://protect2.fireeye.com/v1/url?k=44f7d169-1b6ce86c-44f791f2-
> 86d2114eab2f-a8ea707c0a15247d&q=1&e=f01c1b77-b865-48b7-88fc-
> 96d7dd0e603e&u=https%3A%2F%2Flists.llvm.org%2Fcgi-
> bin%2Fmailman%2Flistinfo%2Fllvm-dev