This is an archive of the discontinued LLVM Phabricator instance.

Differential D113203

[AMDGPU] Small correction in SITargetLowering::performOrCombine()
ClosedPublic

Authored by kpyzhov on Nov 4 2021, 10:09 AM.

Details

Reviewers
kzhuravl
Commits
rGc9690092c8b7: [AMDGPU] Small correction in SITargetLowering::performOrCombine().
Summary

Using the "LHS" variable in the splitBinaryBitConstantOp() call is not valid since "LHS" and "RHS" may be swapped earlier in the code.

Diff Detail

Unit TestsFailed

TimeTest
470 msx64 debian > Polly.ScopInliner::simple-inline-loop.ll

Event Timeline

kpyzhov created this revision.Nov 4 2021, 10:09 AM
Herald added subscribers: foad, kerbowa, hiraditya and 7 others. · View Herald TranscriptNov 4 2021, 10:09 AM
kpyzhov requested review of this revision.Nov 4 2021, 10:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2021, 10:09 AM
Herald added subscribers: llvm-commits, wdng. · View Herald Transcript
foad added a comment.Nov 4 2021, 10:15 AM

Needs a test.

kzhuravl added a comment.Nov 4 2021, 10:19 AM

Agreed on needing a lit test

Harbormaster completed remote builds in B132489: Diff 384797.Nov 4 2021, 12:33 PM
kpyzhov updated this revision to Diff 385086.Nov 5 2021, 8:29 AM
Harbormaster completed remote builds in B132697: Diff 385086.Nov 5 2021, 9:24 AM
kzhuravl accepted this revision.Nov 10 2021, 11:52 AM

LGTM with a nit below. Thanks!

llvm/test/CodeGen/AMDGPU/combine-reg-or-const.ll
1

Can you auto generate this test before submitting?

This revision is now accepted and ready to land.Nov 10 2021, 11:52 AM
kzhuravl added inline comments.Nov 10 2021, 1:19 PM
llvm/test/CodeGen/AMDGPU/combine-reg-or-const.ll
1

Alternatively, you can add more checks for more context, thanks.

This revision was landed with ongoing or failed builds.Nov 10 2021, 6:07 PM
Closed by commit rGc9690092c8b7: [AMDGPU] Small correction in SITargetLowering::performOrCombine(). (authored by kpyzhov). · Explain Why
This revision was automatically updated to reflect the committed changes.
kpyzhov added a commit: rGc9690092c8b7: [AMDGPU] Small correction in SITargetLowering::performOrCombine()..

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
3 lines
test/
CodeGen/
AMDGPU/
27 lines

Diff 385086

llvm/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 9,468 Lines • ▼ Show 20 Lines if (SrcVT == MVT::i32) {
LowOr, HiBits); LowOr, HiBits);
return DAG.getNode(ISD::BITCAST, SL, MVT::i64, Vec); return DAG.getNode(ISD::BITCAST, SL, MVT::i64, Vec);
} }
} }
const ConstantSDNode *CRHS = dyn_cast<ConstantSDNode>(N->getOperand(1)); const ConstantSDNode *CRHS = dyn_cast<ConstantSDNode>(N->getOperand(1));
if (CRHS) { if (CRHS) {
if (SDValue Split if (SDValue Split
= splitBinaryBitConstantOp(DCI, SDLoc(N), ISD::OR, LHS, CRHS)) = splitBinaryBitConstantOp(DCI, SDLoc(N), ISD::OR,
N->getOperand(0), CRHS))
return Split; return Split;
} }
return SDValue(); return SDValue();
} }
SDValue SITargetLowering::performXorCombine(SDNode *N, SDValue SITargetLowering::performXorCombine(SDNode *N,
DAGCombinerInfo &DCI) const { DAGCombinerInfo &DCI) const {
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-    if (SDValue Split
-          = splitBinaryBitConstantOp(DCI, SDLoc(N), ISD::OR,
-                                     N->getOperand(0), CRHS))
+    if (SDValue Split = splitBinaryBitConstantOp(DCI, SDLoc(N), ISD::OR,
+                                                 N->getOperand(0), CRHS))
Lint: Pre-merge checks: clang-format: please reformat the code ``` - if (SDValue Split - =…
EVT VT = N->getValueType(0); EVT VT = N->getValueType(0);
if (VT != MVT::i64) if (VT != MVT::i64)
return SDValue(); return SDValue();
SDValue LHS = N->getOperand(0); SDValue LHS = N->getOperand(0);
SDValue RHS = N->getOperand(1); SDValue RHS = N->getOperand(1);
const ConstantSDNode *CRHS = dyn_cast<ConstantSDNode>(RHS); const ConstantSDNode *CRHS = dyn_cast<ConstantSDNode>(RHS);
▲ Show 20 Lines • Show All 2,916 Lines • Show Last 20 Lines

llvm/test/CodeGen/AMDGPU/combine-reg-or-const.ll

  • This file was added.
; RUN: llc -mtriple=amdgcn-amd-amdhsa -o - %s | FileCheck %s
kzhuravlUnsubmitted
Not Done
ReplyInline Actions

Can you auto generate this test before submitting?

kzhuravl: Can you auto generate this test before submitting?
kzhuravlUnsubmitted
Not Done
ReplyInline Actions

Alternatively, you can add more checks for more context, thanks.

kzhuravl: Alternatively, you can add more checks for more context, thanks.
; The OR instruction should not be eliminated by the "OR Combine" DAG optimization.
; CHECK-LABEL: _Z11test_kernelPii:
; CHECK: s_or_b32
define protected amdgpu_kernel void @_Z11test_kernelPii(i32 addrspace(1)* nocapture %Ad.coerce, i32 %s) local_unnamed_addr #5 {
entry:
%rem.lhs.trunc = trunc i32 %s to i16
%rem4 = urem i16 %rem.lhs.trunc, 12
%rem.zext = zext i16 %rem4 to i32
%cmp = icmp eq i32 %s, 3
br i1 %cmp, label %if.then, label %if.end
if.then: ; preds = %entry
%idxprom = zext i32 %s to i64
%arrayidx3 = getelementptr inbounds i32, i32 addrspace(1)* %Ad.coerce, i64 %idxprom
%div = lshr i32 %rem.zext, 3
%or = or i32 %rem.zext, 192
%add = add nuw nsw i32 %or, %div
store i32 %add, i32 addrspace(1)* %arrayidx3, align 4
br label %if.end
if.end: ; preds = %if.then, %entry
ret void
}