This is an archive of the discontinued LLVM Phabricator instance.

Differential D58481

[AMDGPU] fix commuted case of sub combine
ClosedPublic

Authored by rampitec on Feb 20 2019, 4:14 PM.

Details

Reviewers
arsenm
Commits
rG42e229e13082: [AMDGPU] fix commuted case of sub combine
rL354543: [AMDGPU] fix commuted case of sub combine

Diff Detail

Event Timeline

rampitec created this revision.Feb 20 2019, 4:14 PM
Herald added subscribers: t-tye, tpr, dstuttard and 5 others. · View Herald TranscriptFeb 20 2019, 4:14 PM
rampitec retitled this revision from PATCH] AMDGPU: fix commuted case of sub combine to [AMDGPU] fix commuted case of sub combine.Feb 20 2019, 4:14 PM
arsenm added inline comments.Feb 20 2019, 5:13 PM
lib/Target/AMDGPU/SIISelLowering.cpp
8621

This swap is hard to follow. It's not immediately clear to me why this would be commuted in != subcarry case. What happens in the sub (subcarry, subcarry) case? I think it would be clearer to just explicitly handle the two cases

8625

This needs a comment with the commuted case

8626–8627

This can use C->isNullValue() instead of getZExtValue

test/CodeGen/AMDGPU/combine-cond-add-sub.ll
151

Maybe add a test where both operands to a sub are subcarry

rampitec marked 2 inline comments as done.Feb 20 2019, 5:17 PM
rampitec added inline comments.
lib/Target/AMDGPU/SIISelLowering.cpp
8621

It really does not matter what is the RHS, it can be anything including subcarry.

test/CodeGen/AMDGPU/combine-cond-add-sub.ll
151

That is really no different from any other instruction but complicates the test a lot since we cannot get subcarry right from the IR.

rampitec updated this revision to Diff 187700.Feb 20 2019, 5:25 PM
rampitec marked 2 inline comments as done.
arsenm accepted this revision.Feb 20 2019, 5:35 PM

LGTM

This revision is now accepted and ready to land.Feb 20 2019, 5:35 PM
rampitec planned changes to this revision.Feb 20 2019, 5:46 PM

Actually I think it is still wrong. In the commuted case carry must be added, not subtracted:

x - (y - 0 - cc) => x - y + cc

I will just disable it for commuted case.

rampitec updated this revision to Diff 187707.Feb 20 2019, 5:56 PM

Disabled commuted case completely.

This revision is now accepted and ready to land.Feb 20 2019, 5:56 PM
arsenm added inline comments.Feb 20 2019, 5:59 PM
test/CodeGen/AMDGPU/combine-cond-add-sub.ll
131–133

This needs operand checks

rampitec updated this revision to Diff 187711.Feb 20 2019, 6:21 PM
rampitec marked an inline comment as done.
Closed by commit rL354543: [AMDGPU] fix commuted case of sub combine (authored by rampitec). · Explain WhyFeb 20 2019, 6:58 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2019, 6:58 PM

Revision Contents

PathSize
lib/
Target/
AMDGPU/
5 lines
test/
CodeGen/
AMDGPU/
22 lines

Diff 187692

lib/Target/AMDGPU/SIISelLowering.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 8,611 Lines • ▼ Show 20 LinesSDValue SITargetLowering::performSubCombine(SDNode *N,
if (VT != MVT::i32) if (VT != MVT::i32)
return SDValue(); return SDValue();
SDLoc SL(N); SDLoc SL(N);
SDValue LHS = N->getOperand(0); SDValue LHS = N->getOperand(0);
SDValue RHS = N->getOperand(1); SDValue RHS = N->getOperand(1);
unsigned Opc = LHS.getOpcode(); unsigned Opc = LHS.getOpcode();
if (Opc != ISD::SUBCARRY) bool NeedCommute = Opc != ISD::SUBCARRY;
if (NeedCommute)
arsenmUnsubmitted
Not Done
ReplyInline Actions

This swap is hard to follow. It's not immediately clear to me why this would be commuted in != subcarry case. What happens in the sub (subcarry, subcarry) case? I think it would be clearer to just explicitly handle the two cases

arsenm: This swap is hard to follow. It's not immediately clear to me why this would be commuted in !=…
rampitecAuthorUnsubmitted
Done
ReplyInline Actions

It really does not matter what is the RHS, it can be anything including subcarry.

rampitec: It really does not matter what is the RHS, it can be anything including subcarry.
std::swap(RHS, LHS); std::swap(RHS, LHS);
if (LHS.getOpcode() == ISD::SUBCARRY) { if (LHS.getOpcode() == ISD::SUBCARRY) {
// sub (subcarry x, 0, cc), y => subcarry x, y, cc // sub (subcarry x, 0, cc), y => subcarry x, y, cc
arsenmUnsubmitted
Done
ReplyInline Actions

This needs a comment with the commuted case

arsenm: This needs a comment with the commuted case
auto C = dyn_cast<ConstantSDNode>(LHS.getOperand(1)); auto C = dyn_cast<ConstantSDNode>(LHS.getOperand(1));
if (!C || C->getZExtValue() != 0) if (!C || C->getZExtValue() != 0)
arsenmUnsubmitted
Done
ReplyInline Actions

This can use C->isNullValue() instead of getZExtValue

arsenm: This can use C->isNullValue() instead of getZExtValue
return SDValue(); return SDValue();
SDValue Args[] = { LHS.getOperand(0), RHS, LHS.getOperand(2) }; SDValue Args[] = { LHS.getOperand(0), RHS, LHS.getOperand(2) };
if (NeedCommute)
std::swap(Args[0], Args[1]);
return DAG.getNode(ISD::SUBCARRY, SDLoc(N), LHS->getVTList(), Args); return DAG.getNode(ISD::SUBCARRY, SDLoc(N), LHS->getVTList(), Args);
} }
return SDValue(); return SDValue();
} }
SDValue SITargetLowering::performAddCarrySubCarryCombine(SDNode *N, SDValue SITargetLowering::performAddCarrySubCarryCombine(SDNode *N,
DAGCombinerInfo &DCI) const { DAGCombinerInfo &DCI) const {
▲ Show 20 Lines • Show All 1,226 Lines • Show Last 20 Lines

test/CodeGen/AMDGPU/combine-cond-add-sub.ll

Show First 20 Lines • Show All 121 Lines • ▼ Show 20 Linesbb:
%cmp = icmp ugt i32 %x, %y %cmp = icmp ugt i32 %x, %y
%ext = sext i1 %cmp to i32 %ext = sext i1 %cmp to i32
%adde = add i32 %v, %ext %adde = add i32 %v, %ext
%sub = sub i32 %adde, %a %sub = sub i32 %adde, %a
store i32 %sub, i32 addrspace(1)* %gep, align 4 store i32 %sub, i32 addrspace(1)* %gep, align 4
ret void ret void
} }
; GCN-LABEL: {{^}}sub_sube2:
; GCN-DAG: v_cmp_gt_u32_e{{32|64}} [[CC:[^,]+]], v{{[0-9]+}}, v{{[0-9]+}}
; GCN-DAG: _load_dword [[V:v[0-9]+]],
; GCN: {{v_subb_co_u32|v_subb_u32}}_e{{32|64}} [[SUB:v[0-9]+]], {{[^,]+}}, v{{[0-9]+}}, [[V]], [[CC]]
arsenmUnsubmitted
Done
ReplyInline Actions

This needs operand checks

arsenm: This needs operand checks
; GCN: v_add_{{[ui]}}32_e32 {{.*}}, 0x64, [[SUB]]
; GCN-NOT: v_cndmask
; GCN-NOT: v_sub
define amdgpu_kernel void @sub_sube2(i32 addrspace(1)* nocapture %arg, i32 %a) {
bb:
%x = tail call i32 @llvm.amdgcn.workitem.id.x()
%y = tail call i32 @llvm.amdgcn.workitem.id.y()
%gep = getelementptr inbounds i32, i32 addrspace(1)* %arg, i32 %x
%v = load i32, i32 addrspace(1)* %gep, align 4
%cmp = icmp ugt i32 %x, %y
%ext = sext i1 %cmp to i32
%adde = add i32 %v, %ext
%sub = sub i32 %adde, %a
%sub2 = sub i32 100, %sub
store i32 %sub2, i32 addrspace(1)* %gep, align 4
ret void
}
arsenmUnsubmitted
Not Done
ReplyInline Actions

Maybe add a test where both operands to a sub are subcarry

arsenm: Maybe add a test where both operands to a sub are subcarry
rampitecAuthorUnsubmitted
Done
ReplyInline Actions

That is really no different from any other instruction but complicates the test a lot since we cannot get subcarry right from the IR.

rampitec: That is really no different from any other instruction but complicates the test a lot since we…
; GCN-LABEL: {{^}}sube_sub: ; GCN-LABEL: {{^}}sube_sub:
; GCN: v_cmp_gt_u32_e{{32|64}} [[CC:[^,]+]], v{{[0-9]+}}, v{{[0-9]+}} ; GCN: v_cmp_gt_u32_e{{32|64}} [[CC:[^,]+]], v{{[0-9]+}}, v{{[0-9]+}}
; GCN: v_subb_u32_e{{32|64}} v{{[0-9]+}}, {{[^,]+}}, v{{[0-9]+}}, v{{[0-9]+}}, [[CC]] ; GCN: v_subb_u32_e{{32|64}} v{{[0-9]+}}, {{[^,]+}}, v{{[0-9]+}}, v{{[0-9]+}}, [[CC]]
; GCN-NOT: v_cndmask ; GCN-NOT: v_cndmask
; GCN-NOT: v_sub ; GCN-NOT: v_sub
; GFX9-LABEL: {{^}}sube_sub: ; GFX9-LABEL: {{^}}sube_sub:
; GFX9: v_subb_co_u32_e{{32|64}} v{{[0-9]+}}, vcc ; GFX9: v_subb_co_u32_e{{32|64}} v{{[0-9]+}}, vcc
▲ Show 20 Lines • Show All 81 Lines • Show Last 20 Lines