Skip to content

Nvptx port LowerBITCAST to SelectionDAG #120903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Nvptx port LowerBITCAST to SelectionDAG #120903

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e1b6fce
Ported LowerBITCAST from NVPTXISelLowering.cpp to SelectionDAG/Legali…
GrumpyPigSkin Dec 22, 2024
3320d55
Removed redundant assert check
GrumpyPigSkin Dec 22, 2024
d7cb133
Addressed Most Code Review Comments
GrumpyPigSkin Dec 22, 2024
b674487
Applied code formatting
GrumpyPigSkin Dec 22, 2024
f8dadde
Generalised bit packing and unpacking
GrumpyPigSkin Dec 23, 2024
a6ca08a
Formatting
GrumpyPigSkin Dec 23, 2024
75a533c
Check for only scalar integer types
GrumpyPigSkin Dec 24, 2024
686f698
Merged function
GrumpyPigSkin Jan 16, 2025
d8a82eb
Formatting
GrumpyPigSkin Jan 16, 2025
e5f7304
Updated comments
GrumpyPigSkin Jan 16, 2025
8d01cbe
Updated failing tests
GrumpyPigSkin Jan 20, 2025
87530e4
Code review fixes
GrumpyPigSkin Jan 22, 2025
7a5cfe4
Applied formatting
GrumpyPigSkin Jan 22, 2025
66bfc99
Addressed code review comments
GrumpyPigSkin Jan 25, 2025
f278d56
Formatting
GrumpyPigSkin Jan 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2174,8 +2174,11 @@ SDValue DAGTypeLegalizer::PromoteIntOp_ATOMIC_STORE(AtomicSDNode *N) {
}

SDValue DAGTypeLegalizer::PromoteIntOp_BITCAST(SDNode *N) {
// This should only occur in unusual situations like bitcasting to an
// x86_fp80, so just turn it into a store+load
// Use the custom lowering.
if (SDValue Res = LowerBitcast(N))
return Res;

// If it fails fall back to the default method
return CreateStackStoreLoad(N->getOperand(0), N->getValueType(0));
}

Expand Down
29 changes: 29 additions & 0 deletions llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,35 @@ SDValue DAGTypeLegalizer::CreateStackStoreLoad(SDValue Op,
return DAG.getLoad(DestVT, dl, Store, StackPtr, MachinePointerInfo(), Align);
}

SDValue DAGTypeLegalizer::LowerBitcast(SDNode *Node) const {
assert(Node->getOpcode() == ISD::BITCAST && "Unexpected opcode!");
// Handle bitcasting from v2i8 without hitting the default promotion
// strategy which goes through stack memory.
EVT FromVT = Node->getOperand(0)->getValueType(0);
if (FromVT != MVT::v2i8)
Copy link
Contributor

@arsenm arsenm Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too specific of a type check for the generic legalizer. This code should be generalized over the types supported

Copy link
Contributor Author

@GrumpyPigSkin GrumpyPigSkin Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other types do you think we should support? Or would this be the preferred method from int->vec and vice versa? ie.

<2 x i8>  to i16
<4 x i8>  to i32
<2 x i16> to i32
; etc

I could see that changing a lot of code though.

return SDValue();

// Pack vector elements into i16 and bitcast to final type
SDLoc DL(Node);
SDValue Vec0 = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, MVT::i8,
Node->getOperand(0), DAG.getIntPtrConstant(0, DL));
SDValue Vec1 = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, MVT::i8,
Node->getOperand(0), DAG.getIntPtrConstant(1, DL));

SDValue Extend0 = DAG.getNode(ISD::ZERO_EXTEND, DL, MVT::i16, Vec0);
SDValue Extend1 = DAG.getNode(ISD::ZERO_EXTEND, DL, MVT::i16, Vec1);

EVT ShiftAmtTy =
TLI.getShiftAmountTy(Extend1.getValueType(), DAG.getDataLayout());
SDValue ShiftConst = DAG.getShiftAmountConstant(8, ShiftAmtTy, DL);
Copy link
Contributor

@arsenm arsenm Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct usage. getShiftAmountConstant internally uses getShiftAmountTy. If you use getShiftAmountTy, you use the type directly to getConstant. Or you could use getShiftAmountConstant with Extend1.getValueType()

Copy link
Contributor Author

@GrumpyPigSkin GrumpyPigSkin Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :) I have updated the code

SDValue AsInt = DAG.getNode(
ISD::OR, DL, MVT::i16, Extend0,
DAG.getNode(ISD::SHL, DL, Extend1.getValueType(), Extend1, ShiftConst));
EVT ToVT = Node->getValueType(0);

return DAG.getBitcast(ToVT, AsInt);
}

/// Replace the node's results with custom code provided by the target and
/// return "true", or do nothing and return "false".
/// The last parameter is FALSE if we are dealing with a node with legal
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ class LLVM_LIBRARY_VISIBILITY DAGTypeLegalizer {
SDValue BitConvertToInteger(SDValue Op);
SDValue BitConvertVectorToIntegerVector(SDValue Op);
SDValue CreateStackStoreLoad(SDValue Op, EVT DestVT);
SDValue LowerBitcast(SDNode *N) const;
bool CustomLowerNode(SDNode *N, EVT VT, bool LegalizeResult);
bool CustomWidenLowerNode(SDNode *N, EVT VT);

Expand Down
26 changes: 1 addition & 25 deletions llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2086,30 +2086,6 @@ NVPTXTargetLowering::LowerCONCAT_VECTORS(SDValue Op, SelectionDAG &DAG) const {
return DAG.getBuildVector(Node->getValueType(0), dl, Ops);
}

SDValue NVPTXTargetLowering::LowerBITCAST(SDValue Op, SelectionDAG &DAG) const {
// Handle bitcasting from v2i8 without hitting the default promotion
// strategy which goes through stack memory.
EVT FromVT = Op->getOperand(0)->getValueType(0);
if (FromVT != MVT::v2i8) {
return Op;
}

// Pack vector elements into i16 and bitcast to final type
SDLoc DL(Op);
SDValue Vec0 = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, MVT::i8,
Op->getOperand(0), DAG.getIntPtrConstant(0, DL));
SDValue Vec1 = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, MVT::i8,
Op->getOperand(0), DAG.getIntPtrConstant(1, DL));
SDValue Extend0 = DAG.getNode(ISD::ZERO_EXTEND, DL, MVT::i16, Vec0);
SDValue Extend1 = DAG.getNode(ISD::ZERO_EXTEND, DL, MVT::i16, Vec1);
SDValue Const8 = DAG.getConstant(8, DL, MVT::i16);
SDValue AsInt = DAG.getNode(
ISD::OR, DL, MVT::i16,
{Extend0, DAG.getNode(ISD::SHL, DL, MVT::i16, {Extend1, Const8})});
EVT ToVT = Op->getValueType(0);
return MaybeBitcast(DAG, DL, ToVT, AsInt);
}

// We can init constant f16x2/v2i16/v4i8 with a single .b32 move. Normally it
// would get lowered as two constant loads and vector-packing move.
// Instead we want just a constant move:
Expand Down Expand Up @@ -2619,7 +2595,7 @@ NVPTXTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
case ISD::BUILD_VECTOR:
return LowerBUILD_VECTOR(Op, DAG);
case ISD::BITCAST:
return LowerBITCAST(Op, DAG);
return SDValue();
Copy link
Collaborator

@RKSimon RKSimon Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this case entirely? You might need to remove any custom setOperationAction entries for BITCAST?

Copy link
Contributor Author

@GrumpyPigSkin GrumpyPigSkin Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the case and the setOperationAction but when I did that it started failing the second test case and applying the stack-based bitcast. I am unsure why it did that so I will look into it further. Unless you have any ideas?

Copy link
Collaborator

@RKSimon RKSimon Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DAGTypeLegalizer::ExpandRes_BITCAST already does what you're after (expand bitcast of vector to scalar integer) in a more general form - maybe start there?

Copy link
Contributor Author

@GrumpyPigSkin GrumpyPigSkin Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I had a look at DAGTypeLegalizer::ExpandRes_BITCAST it is close to what I want but it emits the store/load.
I managed to get the code working for the NVPTX test case by removing the setOperationAction. Please will look at the newer code and see if the idea is on the right track?

case ISD::EXTRACT_SUBVECTOR:
return Op;
case ISD::EXTRACT_VECTOR_ELT:
Expand Down
2 changes: 0 additions & 2 deletions llvm/lib/Target/NVPTX/NVPTXISelLowering.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,6 @@ class NVPTXTargetLowering : public TargetLowering {
const NVPTXSubtarget &STI; // cache the subtarget here
SDValue getParamSymbol(SelectionDAG &DAG, int idx, EVT) const;

SDValue LowerBITCAST(SDValue Op, SelectionDAG &DAG) const;

SDValue LowerBUILD_VECTOR(SDValue Op, SelectionDAG &DAG) const;
SDValue LowerCONCAT_VECTORS(SDValue Op, SelectionDAG &DAG) const;
SDValue LowerEXTRACT_VECTOR_ELT(SDValue Op, SelectionDAG &DAG) const;
Expand Down
Loading