Skip to content
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

[AMDGPU] Always lower s/udiv64 by constant to MUL #100723

Merged
merged 5 commits into from
Aug 2, 2024
Merged

[AMDGPU] Always lower s/udiv64 by constant to MUL #100723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6015afd
Add test for div/rem by constant i64
Pierre-vh Jul 26, 2024
530dc05
[AMDGPU] Always lower s/udiv64 by constant to MUL
Pierre-vh Jul 26, 2024
c7618ae
change heuristics
Pierre-vh Jul 29, 2024
bcd6b9f
remove hook
Pierre-vh Jul 29, 2024
a25c8d6
add s/u div/rem by 2, 64, int32 min & max tests
Pierre-vh Jul 30, 2024
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
14 changes: 12 additions & 2 deletions llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6405,7 +6405,12 @@ SDValue TargetLowering::BuildSDIV(SDNode *N, SelectionDAG &DAG,
if (VT.isVector())
WideVT = EVT::getVectorVT(*DAG.getContext(), WideVT,
VT.getVectorElementCount());
if (isOperationLegalOrCustom(ISD::MUL, WideVT)) {
// Some targets like AMDGPU try to go from SDIV to SDIVREM which is then
// custom lowered. This is very expensive so avoid it at all costs for
// constant divisors.
if ((isOperationExpand(ISD::SDIV, VT) &&
isOperationCustom(ISD::SDIVREM, VT.getScalarType())) ||
Comment on lines +6411 to +6412
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic probably isn't exactly right; it probably shouldn't be looking at the scalar type for no reason. However the DAG legalization rules don't really let you figure out if a vector operation is going to be scalarized or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't especially like it, it feels very targeted, but I also couldn't find any alternative that didn't cause crashes or regressions in other backends.

Checking the scalar type is indeed because the flow I observed is (vector SDIV) -> (scalar SDIV) -> (scalar SDVIREM)

isOperationLegalOrCustom(ISD::MUL, WideVT)) {
X = DAG.getNode(ISD::SIGN_EXTEND, dl, WideVT, X);
Y = DAG.getNode(ISD::SIGN_EXTEND, dl, WideVT, Y);
Y = DAG.getNode(ISD::MUL, dl, WideVT, X, Y);
Expand Down Expand Up @@ -6588,7 +6593,12 @@ SDValue TargetLowering::BuildUDIV(SDNode *N, SelectionDAG &DAG,
if (VT.isVector())
WideVT = EVT::getVectorVT(*DAG.getContext(), WideVT,
VT.getVectorElementCount());
if (isOperationLegalOrCustom(ISD::MUL, WideVT)) {
// Some targets like AMDGPU try to go from UDIV to UDIVREM which is then
// custom lowered. This is very expensive so avoid it at all costs for
// constant divisors.
if ((isOperationExpand(ISD::UDIV, VT) &&
isOperationCustom(ISD::UDIVREM, VT.getScalarType())) ||
isOperationLegalOrCustom(ISD::MUL, WideVT)) {
X = DAG.getNode(ISD::ZERO_EXTEND, dl, WideVT, X);
Y = DAG.getNode(ISD::ZERO_EXTEND, dl, WideVT, Y);
Y = DAG.getNode(ISD::MUL, dl, WideVT, X, Y);
Expand Down
Loading
Loading