This is an archive of the discontinued LLVM Phabricator instance.

Differential D81260

[AMDGPU][NFC] Skip processing intrinsics that do not become real instructions
ClosedPublic

Authored by dfukalov on Jun 5 2020, 6:47 AM.

Details

Reviewers
rampitec
Commits
rG6a31a9a5431f: [AMDGPU][NFC] Skip processing intrinsics that do not become real instructions

Diff Detail

Event Timeline

dfukalov created this revision.Jun 5 2020, 6:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 6:47 AM
Herald added subscribers: kerbowa, hiraditya, t-tye and 8 others. · View Herald Transcript
Harbormaster failed remote builds in B59243: Diff 268779!Jun 5 2020, 7:45 AM
dfukalov updated this revision to Diff 268858.Jun 5 2020, 9:58 AM

fixed typo

Harbormaster failed remote builds in B59283: Diff 268858!Jun 5 2020, 10:04 AM
arsenm added a comment.Jun 5 2020, 10:12 AM

Does this solve any concrete problem? If so it needs a testcase

rampitec added a comment.Jun 5 2020, 10:39 AM
This comment was removed by rampitec.
rampitec added a comment.Jun 5 2020, 10:41 AM
This comment was removed by rampitec.
rampitec added a comment.Jun 5 2020, 10:42 AM
In D81260#2076885, @arsenm wrote:

Does this solve any concrete problem? If so it needs a testcase

I agree, it needs a test.

dfukalov retitled this revision from [AMDGPU] Skip processing intrinsics that do not become real instructions to [AMDGPU][NFC] Skip processing intrinsics that do not become real instructions.Jun 5 2020, 12:35 PM
dfukalov added a comment.EditedJun 5 2020, 12:38 PM

Phabricator somehow shows wrong diff after patch update. Correct diff is

-       if (!CI) continue;
+       if (!CI || isa<DbgInfoIntrinsic>(CI) || CI->isLifetimeStartOrEnd())
+         continue;

There were no issues, I just noticed in debug output that that pass tries to process staff like llvm.lifetime.start

arsenm added inline comments.Jun 5 2020, 1:00 PM
llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp
1729

Can also use instructionsWithoutDebug iterator instead

rampitec added a comment.Jun 5 2020, 1:02 PM

But how did it work?! It should have skipped any call which is not a lifetime intrinsic.

dfukalov added a comment.Jun 5 2020, 2:47 PM
In D81260#2077371, @rampitec wrote:

But how did it work?! It should have skipped any call which is not a lifetime intrinsic.

It parsed function name and realized that it is not a mangled library function

rampitec added a comment.Jun 5 2020, 3:15 PM
In D81260#2077613, @dfukalov wrote:
In D81260#2077371, @rampitec wrote:

But how did it work?! It should have skipped any call which is not a lifetime intrinsic.

It parsed function name and realized that it is not a mangled library function

I assume it never did anything to a library function on which it were supposed to work, not since that check was added.

Which means we have no single test for that. Which in turn means we need to add this test now.

dfukalov updated this revision to Diff 268973.Jun 5 2020, 5:43 PM

test added

dfukalov updated this revision to Diff 268974.Jun 5 2020, 5:48 PM

added test

dfukalov marked 2 inline comments as done.Jun 5 2020, 5:55 PM
This comment was removed by dfukalov.
llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp
1729

We cannot use simple range iterator here since BB can be modified in the loop

dfukalov updated this revision to Diff 268979.Jun 5 2020, 6:01 PM
dfukalov marked an inline comment as done.

diff reuploaded

Harbormaster failed remote builds in B59348: Diff 268973!Jun 5 2020, 6:27 PM
Harbormaster failed remote builds in B59349: Diff 268974!
Harbormaster failed remote builds in B59350: Diff 268979!Jun 5 2020, 6:59 PM
rampitec added a comment.Jun 5 2020, 9:13 PM

Hmm... I did not mean that. I meant we seem to miss a test the pass does what it expected to do, not that it skips what it should not process.

dfukalov added a comment.Jun 6 2020, 12:31 AM

There are the tests in llvm/test/CodeGen/AMDGPU/simplify-libcalls.ll

rampitec added a comment.Jun 6 2020, 1:12 AM
In D81260#2078042, @dfukalov wrote:

There are the tests in llvm/test/CodeGen/AMDGPU/simplify-libcalls.ll

How does that work? This is a simple thing: this code shall never been executed given the reverted condition. Your change is absolutely right, but what did it start to execute which so obviously was not executed and tested before?

rampitec accepted this revision.Jun 8 2020, 11:10 AM

OK, I was looking into different versions of this diff. LGTM.

This revision is now accepted and ready to land.Jun 8 2020, 11:10 AM
Closed by commit rG6a31a9a5431f: [AMDGPU][NFC] Skip processing intrinsics that do not become real instructions (authored by dfukalov). · Explain WhyJun 8 2020, 6:17 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
4 lines
test/
CodeGen/
AMDGPU/
24 lines

Diff 269390

llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp

Show First 20 Lines • Show All 1,720 Lines • ▼ Show 20 Linesbool AMDGPUSimplifyLibCalls::runOnFunction(Function &F) {
bool Changed = false; bool Changed = false;
auto AA = &getAnalysis<AAResultsWrapperPass>().getAAResults(); auto AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
LLVM_DEBUG(dbgs() << "AMDIC: process function "; LLVM_DEBUG(dbgs() << "AMDIC: process function ";
F.printAsOperand(dbgs(), false, F.getParent()); dbgs() << '\n';); F.printAsOperand(dbgs(), false, F.getParent()); dbgs() << '\n';);
for (auto &BB : F) { for (auto &BB : F) {
for (BasicBlock::iterator I = BB.begin(), E = BB.end(); I != E; ) { for (BasicBlock::iterator I = BB.begin(), E = BB.end(); I != E; ) {
arsenmUnsubmitted
Done
ReplyInline Actions

Can also use instructionsWithoutDebug iterator instead

arsenm: Can also use instructionsWithoutDebug iterator instead
dfukalovAuthorUnsubmitted
Done
ReplyInline Actions

We cannot use simple range iterator here since BB can be modified in the loop

dfukalov: We cannot use simple range iterator here since BB can be modified in the loop
// Ignore non-calls. // Ignore non-calls.
CallInst *CI = dyn_cast<CallInst>(I); CallInst *CI = dyn_cast<CallInst>(I);
++I; ++I;
if (!CI) continue; // Ignore intrinsics that do not become real instructions.
if (!CI || isa<DbgInfoIntrinsic>(CI) || CI->isLifetimeStartOrEnd())
continue;
// Ignore indirect calls. // Ignore indirect calls.
Function *Callee = CI->getCalledFunction(); Function *Callee = CI->getCalledFunction();
if (Callee == 0) continue; if (Callee == 0) continue;
LLVM_DEBUG(dbgs() << "AMDIC: try folding " << *CI << "\n"; LLVM_DEBUG(dbgs() << "AMDIC: try folding " << *CI << "\n";
dbgs().flush()); dbgs().flush());
if(Simplifier.fold(CI, AA)) if(Simplifier.fold(CI, AA))
Show All 28 Lines

llvm/test/CodeGen/AMDGPU/simplify-libcalls2.ll

  • This file was added.
; REQUIRES: asserts
; RUN: opt -S -amdgpu-simplifylib -debug-only=amdgpu-simplifylib -mtriple=amdgcn-unknown-amdhsa -disable-output < %s 2>&1 | FileCheck %s
; CHECK-NOT: AMDIC: try folding call void @llvm.lifetime.start.p0i8
; CHECK-NOT: AMDIC: try folding call void @llvm.lifetime.end.p0i8
; CHECK-NOT: AMDIC: try folding call void @llvm.dbg.value
define void @foo(i32 %i) {
call void @llvm.lifetime.start.p0i8(i64 1, i8* undef)
call void @llvm.lifetime.end.p0i8(i64 1, i8* undef)
call void @llvm.dbg.value(metadata i32 undef, metadata !DILocalVariable(name: "1", scope: !2), metadata !DIExpression()), !dbg !3
ret void
}
declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
declare void @llvm.dbg.value(metadata, metadata, metadata)
!llvm.module.flags = !{!1}
!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !DIFile(filename: "1", directory: "1"))
!1 = !{i32 2, !"Debug Info Version", i32 3}
!2 = distinct !DISubprogram(unit: !0)
!3 = !DILocation(line: 1, column: 1, scope: !2)