This is an archive of the discontinued LLVM Phabricator instance.

Differential D90391

[gvn] PRE needs to skip convergent intrinsics/calls.
ClosedPublic

Authored by hliao on Oct 29 2020, 6:26 AM.

Details

Reviewers
fhahn
nikic
mkazantsev
jdoerfert
efriedma
Commits
rGc82403d025f3: [gvn] PRE needs to skip convergent intrinsics/calls.
Summary
  • As convergent intrinsics/calls could only be moved to control-equivalent blocks, or more precisely the same divergent branch, PRE needs to skip them.

Diff Detail

Unit TestsFailed

TimeTest
420 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp

Event Timeline

hliao created this revision.Oct 29 2020, 6:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2020, 6:26 AM
Herald added subscribers: llvm-commits, hiraditya. · View Herald Transcript
hliao requested review of this revision.Oct 29 2020, 6:26 AM
hliao updated this revision to Diff 301599.Oct 29 2020, 6:38 AM

Revise the comment.

Harbormaster completed remote builds in B76902: Diff 301597.Oct 29 2020, 7:00 AM
Harbormaster completed remote builds in B76904: Diff 301599.Oct 29 2020, 7:16 AM
mkazantsev added inline comments.Oct 29 2020, 10:15 PM
llvm/test/Transforms/GVN/pre-skip-convergent.ll
2

Please commit this test without your patch with auto-generated checks (utils/update_test_checks.py), then rebase on top of it to show what your patch changes.

jdoerfert added inline comments.Oct 29 2020, 10:31 PM
llvm/test/Transforms/GVN/pre-skip-convergent.ll
2

You probably also want to add a run line for the new pass manager -passes=..., assuming we run this code in the new PM. (Ignore this if we don't).

hliao updated this revision to Diff 301900.Oct 30 2020, 8:02 AM

Revise the test following reviewers' comments.

hliao marked 2 inline comments as done.Oct 30 2020, 8:02 AM
Harbormaster completed remote builds in B77046: Diff 301900.Oct 30 2020, 8:02 AM
hliao added inline comments.Oct 30 2020, 8:05 AM
llvm/test/Transforms/GVN/pre-skip-convergent.ll
2

with this patch, we won't PRE that call into convergent intrinsic @llvm.convergent in merge block into pre block as it won't function correctly.

jdoerfert accepted this revision.Oct 30 2020, 8:14 AM

LGTM

This revision is now accepted and ready to land.Oct 30 2020, 8:14 AM
This revision was landed with ongoing or failed builds.Oct 30 2020, 8:24 AM
Closed by commit rGc82403d025f3: [gvn] PRE needs to skip convergent intrinsics/calls. (authored by hliao). · Explain Why
This revision was automatically updated to reflect the committed changes.
hliao added a commit: rGc82403d025f3: [gvn] PRE needs to skip convergent intrinsics/calls..

Revision Contents

PathSize
llvm/
lib/
Transforms/
Scalar/
6 lines
test/
Transforms/
GVN/
25 lines

Diff 301597

llvm/lib/Transforms/Scalar/GVN.cpp

Show First 20 Lines • Show All 2,455 Lines • ▼ Show 20 Linesbool GVN::performScalarPRE(Instruction *CurInst) {
// introduce unnecessary spills. // introduce unnecessary spills.
// //
// This doesn't prevent Load PRE. PHI translation will make the GEP available // This doesn't prevent Load PRE. PHI translation will make the GEP available
// to the load by moving it to the predecessor block if necessary. // to the load by moving it to the predecessor block if necessary.
if (isa<GetElementPtrInst>(CurInst)) if (isa<GetElementPtrInst>(CurInst))
return false; return false;
// We don't currently value number ANY inline asm calls. // We don't currently value number ANY inline asm calls.
if (auto *CallB = dyn_cast<CallBase>(CurInst)) if (auto *CallB = dyn_cast<CallBase>(CurInst)) {
if (CallB->isInlineAsm()) if (CallB->isInlineAsm())
return false; return false;
// Don't do PRE on convergent calls.
if (CallB->isConvergent())
return false;
}
uint32_t ValNo = VN.lookup(CurInst); uint32_t ValNo = VN.lookup(CurInst);
// Look for the predecessors for PRE opportunities. We're // Look for the predecessors for PRE opportunities. We're
// only trying to solve the basic diamond case, where // only trying to solve the basic diamond case, where
// a value is computed in the successor and one predecessor, // a value is computed in the successor and one predecessor,
// but not the other. We also explicitly disallow cases // but not the other. We also explicitly disallow cases
// where the successor is its own predecessor, because they're // where the successor is its own predecessor, because they're
▲ Show 20 Lines • Show All 441 Lines • Show Last 20 Lines

llvm/test/Transforms/GVN/pre-skip-convergent.ll

  • This file was added.
; RUN: opt -S -gvn -o - %s | FileCheck %s
mkazantsevUnsubmitted
Done
ReplyInline Actions

Please commit this test without your patch with auto-generated checks (utils/update_test_checks.py), then rebase on top of it to show what your patch changes.

mkazantsev: Please commit this test without your patch with auto-generated checks…
jdoerfertUnsubmitted
Done
ReplyInline Actions

You probably also want to add a run line for the new pass manager -passes=..., assuming we run this code in the new PM. (Ignore this if we don't).

jdoerfert: You probably also want to add a run line for the new pass manager `-passes=...`, assuming we…
hliaoAuthorUnsubmitted
Done
ReplyInline Actions

with this patch, we won't PRE that call into convergent intrinsic @llvm.convergent in merge block into pre block as it won't function correctly.

hliao: with this patch, we won't PRE that call into convergent intrinsic `@llvm.convergent` in `merge`…
; CHECK-LABEL: @foo
; CHECK: pre:
; CHECK-NOT: call i32 @llvm.convergent
; CHECK: merge:
define i32 @foo(i1 %cond, i32* %q, i32* %p) {
entry:
%v0 = call i32 @llvm.convergent(i32 0)
store i32 %v0, i32* %q
br i1 %cond, label %pre, label %merge
pre:
%t0 = load i32, i32* %p
br label %merge
merge:
%m0 = phi i32 [ %t0, %pre ], [ 0, %entry ]
%r0 = call i32 @llvm.convergent(i32 %m0)
ret i32 %r0
}
declare i32 @llvm.convergent(i32) #0
attributes #0 = { convergent nounwind readnone }