[SelectionDAG] Make SSHLSAT/USHLSAT obey getShiftAmountTy(). #173216
+41 −21
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Treat these like other shift operations by allowing the shift amount to be a different type than the result. The PromoteIntOp_Shift and LegalizeDAG code are not tested due to lack of target support. I'm looking at adding SSHLSAT for the RISC-V P extension. I don't need this support for that. I just thought it was weird that they weren't like other shifts.
Member
| @llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-backend-x86 Author: Craig Topper (topperc) ChangesTreat these like other shift operations by allowing the shift amount to be a different type than the result. The PromoteIntOp_Shift and LegalizeDAG code are not tested due to lack of target support. I'm looking at adding SSHLSAT for the RISC-V P extension. I don't need this support for that. I just thought it was weird that they weren't like other shifts. Full diff: https://github.com/llvm/llvm-project/pull/173216.diff 9 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/ISDOpcodes.h b/llvm/include/llvm/CodeGen/ISDOpcodes.h index 437cf1a6cf089..27a9019a258f8 100644 --- a/llvm/include/llvm/CodeGen/ISDOpcodes.h +++ b/llvm/include/llvm/CodeGen/ISDOpcodes.h @@ -371,11 +371,13 @@ enum NodeType { /// RESULT = [US]SHLSAT(LHS, RHS) - Perform saturation left shift. The first /// operand is the value to be shifted, and the second argument is the amount - /// to shift by. Both must be integers of the same bit width (W). If the true - /// value of LHS << RHS exceeds the largest value that can be represented by - /// W bits, the resulting value is this maximum value, Otherwise, if this - /// value is less than the smallest value that can be represented by W bits, - /// the resulting value is this minimum value. + /// to shift by. Both must be integers. After legalization the type of the + /// shift amount is known to be TLI.getShiftAmountTy(). Before legalization + /// the shift amount can be any type, but care must be taken to ensure it is + /// large enough. If the true value of LHS << RHS exceeds the largest value + /// that can be represented by W bits, the resulting value is this maximum + /// value, Otherwise, if this value is less than the smallest value that can + /// be represented by W bits, the resulting value is this minimum value. SSHLSAT, USHLSAT, diff --git a/llvm/include/llvm/Target/TargetSelectionDAG.td b/llvm/include/llvm/Target/TargetSelectionDAG.td index e6f14b9f1a402..994bb61697617 100644 --- a/llvm/include/llvm/Target/TargetSelectionDAG.td +++ b/llvm/include/llvm/Target/TargetSelectionDAG.td @@ -478,8 +478,8 @@ def saddsat : SDNode<"ISD::SADDSAT" , SDTIntBinOp, [SDNPCommutative]>; def uaddsat : SDNode<"ISD::UADDSAT" , SDTIntBinOp, [SDNPCommutative]>; def ssubsat : SDNode<"ISD::SSUBSAT" , SDTIntBinOp>; def usubsat : SDNode<"ISD::USUBSAT" , SDTIntBinOp>; -def sshlsat : SDNode<"ISD::SSHLSAT" , SDTIntBinOp>; -def ushlsat : SDNode<"ISD::USHLSAT" , SDTIntBinOp>; +def sshlsat : SDNode<"ISD::SSHLSAT" , SDTIntShiftOp>; +def ushlsat : SDNode<"ISD::USHLSAT" , SDTIntShiftOp>; def smulfix : SDNode<"ISD::SMULFIX" , SDTIntScaledBinOp, [SDNPCommutative]>; def smulfixsat : SDNode<"ISD::SMULFIXSAT", SDTIntScaledBinOp, [SDNPCommutative]>; diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp index 73f59b4064144..62ff7a437dde2 100644 --- a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp @@ -1291,7 +1291,9 @@ void SelectionDAGLegalize::LegalizeOp(SDNode *Node) { case ISD::SRL: case ISD::SRA: case ISD::ROTL: - case ISD::ROTR: { + case ISD::ROTR: + case ISD::SSHLSAT: + case ISD::USHLSAT: { // Legalizing shifts/rotates requires adjusting the shift amount // to the appropriate width. SDValue Op0 = Node->getOperand(0); diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp index 981db6d90a8f6..1152134d8db3d 100644 --- a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp @@ -1144,7 +1144,8 @@ SDValue DAGTypeLegalizer::PromoteIntRes_ADDSUBSHLSAT(SDNode *N) { // FIXME: We need vp-aware PromotedInteger functions. if (IsShift) { Op1 = GetPromotedInteger(Op1); - Op2 = ZExtPromotedInteger(Op2); + if (getTypeAction(Op2.getValueType()) == TargetLowering::TypePromoteInteger) + Op2 = ZExtPromotedInteger(Op2); } else { Op1 = SExtPromotedInteger(Op1); Op2 = SExtPromotedInteger(Op2); @@ -2052,7 +2053,9 @@ bool DAGTypeLegalizer::PromoteIntegerOperand(SDNode *N, unsigned OpNo) { case ISD::SRA: case ISD::SRL: case ISD::ROTL: - case ISD::ROTR: Res = PromoteIntOp_Shift(N); break; + case ISD::ROTR: + case ISD::SSHLSAT: + case ISD::USHLSAT: Res = PromoteIntOp_Shift(N); break; case ISD::SCMP: case ISD::UCMP: Res = PromoteIntOp_CMP(N); break; diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp index 4482df15242d9..891f584cf0c3a 100644 --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp @@ -7826,6 +7826,8 @@ SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT, [[fallthrough]]; case ISD::ROTL: case ISD::ROTR: + case ISD::SSHLSAT: + case ISD::USHLSAT: assert(VT == N1.getValueType() && "Shift operators return type must be the same as their first arg"); assert(VT.isInteger() && N2.getValueType().isInteger() && diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp index b8da6010bb74d..9e342f9c4416f 100644 --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp @@ -7343,16 +7343,26 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I, setValue(&I, DAG.getNode(ISD::USUBSAT, sdl, Op1.getValueType(), Op1, Op2)); return; } - case Intrinsic::sshl_sat: { - SDValue Op1 = getValue(I.getArgOperand(0)); - SDValue Op2 = getValue(I.getArgOperand(1)); - setValue(&I, DAG.getNode(ISD::SSHLSAT, sdl, Op1.getValueType(), Op1, Op2)); - return; - } + case Intrinsic::sshl_sat: case Intrinsic::ushl_sat: { SDValue Op1 = getValue(I.getArgOperand(0)); SDValue Op2 = getValue(I.getArgOperand(1)); - setValue(&I, DAG.getNode(ISD::USHLSAT, sdl, Op1.getValueType(), Op1, Op2)); + + EVT ShiftTy = DAG.getTargetLoweringInfo().getShiftAmountTy( + Op1.getValueType(), DAG.getDataLayout()); + + // Coerce the shift amount to the right type if we can. This exposes the + // truncate or zext to optimization early. + if (!I.getType()->isVectorTy() && Op2.getValueType() != ShiftTy) { + assert(ShiftTy.getSizeInBits() >= + Log2_32_Ceil(Op1.getValueSizeInBits()) && + "Unexpected shift type"); + Op2 = DAG.getZExtOrTrunc(Op2, getCurSDLoc(), ShiftTy); + } + + unsigned Opc = + Intrinsic == Intrinsic::sshl_sat ? ISD::SSHLSAT : ISD::USHLSAT; + setValue(&I, DAG.getNode(Opc, sdl, Op1.getValueType(), Op1, Op2)); return; } case Intrinsic::smul_fix: diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp index 29e13c558fca8..e8fb3b7659c44 100644 --- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp @@ -11054,7 +11054,6 @@ SDValue TargetLowering::expandShlSat(SDNode *Node, SelectionDAG &DAG) const { assert((Node->getOpcode() == ISD::SSHLSAT || Node->getOpcode() == ISD::USHLSAT) && "Expected a SHLSAT opcode"); - assert(VT == RHS.getValueType() && "Expected operands to be the same type"); assert(VT.isInteger() && "Expected operands to be integers"); if (VT.isVector() && !isOperationLegalOrCustom(ISD::VSELECT, VT)) diff --git a/llvm/test/CodeGen/X86/sshl_sat.ll b/llvm/test/CodeGen/X86/sshl_sat.ll index a93be22bf5861..469b62453f37f 100644 --- a/llvm/test/CodeGen/X86/sshl_sat.ll +++ b/llvm/test/CodeGen/X86/sshl_sat.ll @@ -179,9 +179,9 @@ define i4 @func4(i4 %x, i4 %y) nounwind { ; X86-LABEL: func4: ; X86: # %bb.0: ; X86-NEXT: pushl %esi +; X86-NEXT: movzbl {{[0-9]+}}(%esp), %edx ; X86-NEXT: movzbl {{[0-9]+}}(%esp), %ecx ; X86-NEXT: andb $15, %cl -; X86-NEXT: movzbl {{[0-9]+}}(%esp), %edx ; X86-NEXT: shlb $4, %dl ; X86-NEXT: movb %dl, %ch ; X86-NEXT: shlb %cl, %ch diff --git a/llvm/test/CodeGen/X86/ushl_sat.ll b/llvm/test/CodeGen/X86/ushl_sat.ll index 9768e4761f47a..9985ac61a4e54 100644 --- a/llvm/test/CodeGen/X86/ushl_sat.ll +++ b/llvm/test/CodeGen/X86/ushl_sat.ll @@ -156,9 +156,9 @@ define i4 @func4(i4 %x, i4 %y) nounwind { ; X86-LABEL: func4: ; X86: # %bb.0: ; X86-NEXT: pushl %esi +; X86-NEXT: movzbl {{[0-9]+}}(%esp), %eax ; X86-NEXT: movzbl {{[0-9]+}}(%esp), %ecx ; X86-NEXT: andb $15, %cl -; X86-NEXT: movzbl {{[0-9]+}}(%esp), %eax ; X86-NEXT: shlb $4, %al ; X86-NEXT: movl %eax, %edx ; X86-NEXT: shlb %cl, %dl |
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,h -- llvm/include/llvm/CodeGen/ISDOpcodes.h llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp index e8fb3b765..d5daba9f5 100644 --- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp @@ -11053,7 +11053,7 @@ SDValue TargetLowering::expandShlSat(SDNode *Node, SelectionDAG &DAG) const { assert((Node->getOpcode() == ISD::SSHLSAT || Node->getOpcode() == ISD::USHLSAT) && - "Expected a SHLSAT opcode"); + "Expected a SHLSAT opcode"); assert(VT.isInteger() && "Expected operands to be integers"); if (VT.isVector() && !isOperationLegalOrCustom(ISD::VSELECT, VT)) |
RKSimon approved these changes Dec 22, 2025
Collaborator
RKSimon left a comment
RKSimon left a comment There was a problem hiding this comment.
LGTM by inspection - please fix the clang-format CI warning
It'd be better to have test coverage in riscv though since this is the main target that supports saturated shift afaict
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Treat these like other shift operations by allowing the shift amount to be a different type than the result.
The PromoteIntOp_Shift and LegalizeDAG code are not tested due to lack of target support.
I'm looking at adding SSHLSAT for the RISC-V P extension. I don't need this support for that. I just thought it was weird that they weren't like other shifts.