This is an archive of the discontinued LLVM Phabricator instance.

Differential D38466

[ TargetLowering, AMDGPU] Use the return value of UpdateNodeOperands();
ClosedPublic

Authored by msearles on Oct 2 2017, 9:57 AM.

Details

Reviewers
wdng
craig.topper
RKSimon
arsenm
Commits
rG4e3d6160db32: Use the return value of UpdateNodeOperands(); in some cases, UpdateNodeOperands…
rL315957: Use the return value of UpdateNodeOperands(); in some cases, UpdateNodeOperands…
Summary

Use the return value of UpdateNodeOperands(); in some cases, UpdateNodeOperands() modifies the node in-place and using the return value isn’t strictly necessary. However, it does not necessarily modify the node, but may return a resultant node if it already exists in the DAG. See comments in UpdateNodeOperands(). In that case, the return value must be used to avoid such scenarios as an infinite loop (node is assumed to have been updated, so added back to the worklist, and re-processed; however, node hasn’t changed so it is once again passed to UpdateNodeOperands(), assumed modified, added back to worklist; cycle infinitely repeats).

Diff Detail

Event Timeline

msearles created this revision.Oct 2 2017, 9:57 AM
Herald added subscribers: tpr, nhaehnle, arsenm. · View Herald TranscriptOct 2 2017, 9:57 AM
msearles retitled this revision from Use the return value of UpdateNodeOperands(); to [ TargetLowering, AMDGPU] Use the return value of UpdateNodeOperands(); .Oct 2 2017, 10:01 AM
msearles edited the summary of this revision. (Show Details)
craig.topper added a reviewer: RKSimon.Oct 2 2017, 10:03 AM
arsenm added inline comments.Oct 2 2017, 10:29 AM
test/CodeGen/AMDGPU/simplifydemandedbits-recursion.ll
2

Does this actually work? I've never seen another test use ulimit or a sub shell. 5 seconds seems high also.

75

Run instnamer on this test

RKSimon added inline comments.Oct 2 2017, 10:45 AM
lib/Target/AMDGPU/SIISelLowering.cpp
6488
return DAG.UpdateNodeOperands(Node, Ops);
test/CodeGen/AMDGPU/simplifydemandedbits-recursion.ll
2

Does this work on windows builds?

17

You're not testing that foo() is empty - just that it includes s_endpgm

msearles updated this revision to Diff 117408.Oct 2 2017, 12:43 PM

Update per reviewer comments

Herald added subscribers: t-tye, dstuttard, yaxunl, kzhuravl. · View Herald TranscriptOct 2 2017, 12:43 PM
msearles marked 5 inline comments as done.Oct 2 2017, 12:46 PM
msearles added inline comments.
test/CodeGen/AMDGPU/simplifydemandedbits-recursion.ll
2

Nope; will not work on Windows; test adjusted.

2

Yes, it works quite nicely on Linux; it will not work on Windows (thanks Simon) as ulimit is not available. I removed the ulimit. So, potentially, there may be a compiler regression that causes this to loop infinitely; however, I think that you could say that about any test.

17

Updated the comments within the test; the purpose of the test is to verify that code was generated, not specifically that foo() is empty.

75

Done.

msearles added a reviewer: arsenm.Oct 9 2017, 1:20 PM
msearles marked 2 inline comments as done.

Ping

RKSimon added inline comments.Oct 9 2017, 1:51 PM
test/CodeGen/AMDGPU/simplifydemandedbits-recursion.ll
2

Remove the brackets? Also, drop the prefix and use the default 'CHECK' ?

msearles updated this revision to Diff 118274.Oct 9 2017, 2:36 PM

Update test per reviewer comments.

msearles marked an inline comment as done.Oct 9 2017, 2:37 PM
msearles added inline comments.
test/CodeGen/AMDGPU/simplifydemandedbits-recursion.ll
2

Done and done.

RKSimon edited edge metadata.Oct 10 2017, 4:33 AM

Where did the code change go in the diff?

msearles updated this revision to Diff 118396.Oct 10 2017, 8:06 AM
msearles marked an inline comment as done.

Added correct diff file.

msearles added a comment.Oct 13 2017, 10:50 AM

Ping

arsenm accepted this revision.Oct 16 2017, 2:43 PM

LGTM

This revision is now accepted and ready to land.Oct 16 2017, 2:43 PM
Closed by commit rL315957: Use the return value of UpdateNodeOperands(); in some cases, UpdateNodeOperands… (authored by msearles). · Explain WhyOct 16 2017, 4:39 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
CodeGen/
SelectionDAG/
2 lines
Target/
AMDGPU/
3 lines
test/
CodeGen/
AMDGPU/
95 lines

Diff 118396

lib/CodeGen/SelectionDAG/TargetLowering.cpp

Show First 20 Lines • Show All 463 Lines • ▼ Show 20 LinesTargetLowering::SimplifyDemandedBits(SDNode *User, unsigned OpIdx,
SmallVector <SDValue, 4> NewOps; SmallVector <SDValue, 4> NewOps;
for (unsigned i = 0, e = User->getNumOperands(); i != e; ++i) { for (unsigned i = 0, e = User->getNumOperands(); i != e; ++i) {
if (i == OpIdx) { if (i == OpIdx) {
NewOps.push_back(TLO.New); NewOps.push_back(TLO.New);
continue; continue;
} }
NewOps.push_back(User->getOperand(i)); NewOps.push_back(User->getOperand(i));
} }
TLO.DAG.UpdateNodeOperands(User, NewOps); User = TLO.DAG.UpdateNodeOperands(User, NewOps);
// Op has less users now, so we may be able to perform additional combines // Op has less users now, so we may be able to perform additional combines
// with it. // with it.
DCI.AddToWorklist(Op.getNode()); DCI.AddToWorklist(Op.getNode());
// User's operands have been updated, so we may be able to do new combines // User's operands have been updated, so we may be able to do new combines
// with it. // with it.
DCI.AddToWorklist(User); DCI.AddToWorklist(User);
return true; return true;
} }
▲ Show 20 Lines • Show All 3,418 Lines • Show Last 20 Lines

lib/Target/AMDGPU/SIISelLowering.cpp

Show First 20 Lines • Show All 6,479 Lines • ▼ Show 20 Lines for (unsigned i = 0; i < Node->getNumOperands(); ++i) {
} }
SDLoc DL(Node); SDLoc DL(Node);
Ops.push_back(SDValue(DAG.getMachineNode(AMDGPU::S_MOV_B32, DL, Ops.push_back(SDValue(DAG.getMachineNode(AMDGPU::S_MOV_B32, DL,
Node->getOperand(i).getValueType(), Node->getOperand(i).getValueType(),
Node->getOperand(i)), 0)); Node->getOperand(i)), 0));
} }
DAG.UpdateNodeOperands(Node, Ops); return DAG.UpdateNodeOperands(Node, Ops);
RKSimonUnsubmitted
Done
ReplyInline Actions
return DAG.UpdateNodeOperands(Node, Ops);
RKSimon: ``` return DAG.UpdateNodeOperands(Node, Ops); ```
return Node;
} }
/// \brief Fold the instructions after selecting them. /// \brief Fold the instructions after selecting them.
SDNode *SITargetLowering::PostISelFolding(MachineSDNode *Node, SDNode *SITargetLowering::PostISelFolding(MachineSDNode *Node,
SelectionDAG &DAG) const { SelectionDAG &DAG) const {
const SIInstrInfo *TII = getSubtarget()->getInstrInfo(); const SIInstrInfo *TII = getSubtarget()->getInstrInfo();
unsigned Opcode = Node->getMachineOpcode(); unsigned Opcode = Node->getMachineOpcode();
▲ Show 20 Lines • Show All 335 Lines • Show Last 20 Lines

test/CodeGen/AMDGPU/simplifydemandedbits-recursion.ll

  • This file was added.
; RUN: llc -march=amdgcn < %s | FileCheck %s
arsenmUnsubmitted
Done
ReplyInline Actions

Does this actually work? I've never seen another test use ulimit or a sub shell. 5 seconds seems high also.

arsenm: Does this actually work? I've never seen another test use ulimit or a sub shell. 5 seconds…
RKSimonUnsubmitted
Done
ReplyInline Actions

Does this work on windows builds?

RKSimon: Does this work on windows builds?
msearlesAuthorUnsubmitted
Not Done
ReplyInline Actions

Nope; will not work on Windows; test adjusted.

msearles: Nope; will not work on Windows; test adjusted.
msearlesAuthorUnsubmitted
Not Done
ReplyInline Actions

Yes, it works quite nicely on Linux; it will not work on Windows (thanks Simon) as ulimit is not available. I removed the ulimit. So, potentially, there may be a compiler regression that causes this to loop infinitely; however, I think that you could say that about any test.

msearles: Yes, it works quite nicely on Linux; it will not work on Windows (thanks Simon) as ulimit is…
RKSimonUnsubmitted
Done
ReplyInline Actions

Remove the brackets? Also, drop the prefix and use the default 'CHECK' ?

RKSimon: Remove the brackets? Also, drop the prefix and use the default 'CHECK' ?
msearlesAuthorUnsubmitted
Not Done
ReplyInline Actions

Done and done.

msearles: Done and done.
; Check we can compile this bugpoint-reduced test without an
; infinite loop in TLI.SimplifyDemandedBits() due to failure
; to use return value of TLO.DAG.UpdateNodeOperands()
; Check that code was generated; we know there will be
; a s_endpgm, so check for it.
@0 = external unnamed_addr addrspace(3) global [462 x float], align 4
; Function Attrs: nounwind readnone speculatable
declare i32 @llvm.amdgcn.workitem.id.y() #0
; Function Attrs: nounwind readnone speculatable
declare i32 @llvm.amdgcn.workitem.id.x() #0
RKSimonUnsubmitted
Done
ReplyInline Actions

You're not testing that foo() is empty - just that it includes s_endpgm

RKSimon: You're not testing that foo() is empty - just that it includes s_endpgm
msearlesAuthorUnsubmitted
Not Done
ReplyInline Actions

Updated the comments within the test; the purpose of the test is to verify that code was generated, not specifically that foo() is empty.

msearles: Updated the comments within the test; the purpose of the test is to verify that code was…
; Function Attrs: nounwind readnone speculatable
declare float @llvm.fmuladd.f32(float, float, float) #0
; CHECK: s_endpgm
define amdgpu_kernel void @foo(float addrspace(1)* noalias nocapture readonly %arg, float addrspace(1)* noalias nocapture readonly %arg1, float addrspace(1)* noalias nocapture %arg2, float %arg3) local_unnamed_addr !reqd_work_group_size !0 {
bb:
%tmp = tail call i32 @llvm.amdgcn.workitem.id.y()
%tmp4 = tail call i32 @llvm.amdgcn.workitem.id.x()
%tmp5 = and i32 %tmp, 15
%tmp6 = mul nuw nsw i32 %tmp5, 21
%tmp7 = sub i32 %tmp6, 0
%tmp8 = add i32 %tmp7, 0
%tmp9 = add i32 %tmp8, 0
%tmp10 = getelementptr inbounds [462 x float], [462 x float] addrspace(3)* @0, i32 0, i32 0
br label %bb12
bb11: ; preds = %bb30
br i1 undef, label %bb37, label %bb38
bb12: ; preds = %bb30, %bb
br i1 false, label %.preheader, label %.loopexit145
.loopexit145: ; preds = %.preheader, %bb12
br label %bb13
bb13: ; preds = %.loopexit, %.loopexit145
%tmp14 = phi i32 [ %tmp5, %.loopexit145 ], [ %tmp20, %.loopexit ]
%tmp15 = add nsw i32 %tmp14, -3
%tmp16 = mul i32 %tmp14, 21
br i1 undef, label %bb17, label %.loopexit
bb17: ; preds = %bb13
%tmp18 = mul i32 %tmp15, 224
%tmp19 = add i32 undef, %tmp18
br label %bb21
.loopexit: ; preds = %bb21, %bb13
%tmp20 = add nuw nsw i32 %tmp14, 16
br i1 undef, label %bb13, label %bb26
bb21: ; preds = %bb21, %bb17
%tmp22 = phi i32 [ %tmp4, %bb17 ], [ %tmp25, %bb21 ]
%tmp23 = add i32 %tmp22, %tmp16
%tmp24 = getelementptr inbounds float, float addrspace(3)* %tmp10, i32 %tmp23
store float undef, float addrspace(3)* %tmp24, align 4
%tmp25 = add nuw i32 %tmp22, 8
br i1 undef, label %bb21, label %.loopexit
bb26: ; preds = %.loopexit
br label %bb31
.preheader: ; preds = %.preheader, %bb12
%tmp27 = phi i32 [ %tmp28, %.preheader ], [ undef, %bb12 ]
%tmp28 = add nuw i32 %tmp27, 128
%tmp29 = icmp ult i32 %tmp28, 1568
br i1 %tmp29, label %.preheader, label %.loopexit145
bb30: ; preds = %bb31
arsenmUnsubmitted
Done
ReplyInline Actions

Run instnamer on this test

arsenm: Run instnamer on this test
msearlesAuthorUnsubmitted
Not Done
ReplyInline Actions

Done.

msearles: Done.
br i1 undef, label %bb11, label %bb12
bb31: ; preds = %bb31, %bb26
%tmp32 = phi i32 [ %tmp9, %bb26 ], [ undef, %bb31 ]
%tmp33 = getelementptr inbounds [462 x float], [462 x float] addrspace(3)* @0, i32 0, i32 %tmp32
%tmp34 = load float, float addrspace(3)* %tmp33, align 4
%tmp35 = tail call float @llvm.fmuladd.f32(float %tmp34, float undef, float undef)
%tmp36 = tail call float @llvm.fmuladd.f32(float undef, float undef, float %tmp35)
br i1 undef, label %bb30, label %bb31
bb37: ; preds = %bb11
br label %bb38
bb38: ; preds = %bb37, %bb11
ret void
}
attributes #0 = { nounwind readnone speculatable }
!0 = !{i32 8, i32 16, i32 1}