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

Mon Sep 27 09:32:01 PDT 2021

At the very least I've fixed my current patch to take
DAG.getLibInfo().getIntSize() into account:
https://reviews.llvm.org/D110508

I'm not sure regarding the point about ExpandIntRes_Rotate (or rather,
TargetLowering::expandROT);
I do see other methods of TargetLowering that use getShiftAmountTy() (e.g.
expandCTPOP, expandCTLZ),
whereas expandROT simply uses Op1.getValueType().

Itay

On Mon, 27 Sept 2021 at 18:03, Björn Pettersson A <
bjorn.a.pettersson at ericsson.com> wrote:

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

-- 

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210927/52c3b603/attachment.html>