Itay Bookstein via llvm-dev
2021-Sep-26 19:21 UTC
[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.
Björn Pettersson A via llvm-dev
2021-Sep-27 15:03 UTC
[llvm-dev] [SelectionDAG] Wrong type for shift libcalls when expanding funnelled shifts
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