This is an archive of the discontinued LLVM Phabricator instance.

Differential D100188

[AMDGPU] SIFoldOperands: eagerly erase dead REG_SEQUENCEs
ClosedPublic

Authored by foad on Apr 9 2021, 6:11 AM.

Details

Reviewers
arsenm
rampitec
Commits
rG323ef0eb4590: [AMDGPU] SIFoldOperands: eagerly erase dead REG_SEQUENCEs
rGd19a42eba98f: [AMDGPU] SIFoldOperands: eagerly erase dead REG_SEQUENCEs
Summary

This is fairly cheap to implement and means less work for future
passes like MachineDCE.

Diff Detail

Event Timeline

foad created this revision.Apr 9 2021, 6:11 AM
Herald added subscribers: kerbowa, hiraditya, t-tye and 6 others. · View Herald TranscriptApr 9 2021, 6:11 AM
foad requested review of this revision.Apr 9 2021, 6:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 6:11 AM
Herald added subscribers: llvm-commits, wdng. · View Herald Transcript
foad added inline comments.Apr 9 2021, 6:13 AM
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
1547

Do we actually need to follow copies here? I don't see any lit tests regressions if I remove this loop.

foad added a child revision: D100189: [AMDGPU] Remove MachineDCE after SIFoldOperands.Apr 9 2021, 6:13 AM
Harbormaster completed remote builds in B97963: Diff 336427.Apr 9 2021, 6:52 AM
rampitec accepted this revision.Apr 9 2021, 10:44 AM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
1547

The relevant test was in the acc-ldst.ll at least initially, all the subtests with loops. It was written to fold a store from LCSSA PHI. We had too many changes since then though so the test probably lost some of its value, but the code is still relevant.

This revision is now accepted and ready to land.Apr 9 2021, 10:44 AM
Closed by commit rGd19a42eba98f: [AMDGPU] SIFoldOperands: eagerly erase dead REG_SEQUENCEs (authored by foad). · Explain WhyApr 9 2021, 12:42 PM
This revision was automatically updated to reflect the committed changes.
foad added a commit: rGd19a42eba98f: [AMDGPU] SIFoldOperands: eagerly erase dead REG_SEQUENCEs.
hctim added a subscriber: hctim.Apr 9 2021, 3:04 PM

Looks like this patch might have broken the ASan buildbots (https://lab.llvm.org/buildbot/#/builders/99/builds/2766):

Waiting on that build to finish and bisecting locally, but here's the ASan report from one of the relevant test failures. Full repro instructions at
https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild, but I believe this should be reproducible using check-llvm with cmake -DLLVM_USE_SANITIZER=Address.

FAIL: LLVM :: CodeGen/AMDGPU/wwm-reserved.ll (35874 of 75435)
******************** TEST 'LLVM :: CodeGen/AMDGPU/wwm-reserved.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/bin/llc -O0 -march=amdgcn -mcpu=gfx900 -amdgpu-dpp-combine=false -verify-machineinstrs < /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/test/CodeGen/AMDGPU/wwm-reserved.ll | /b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/bin/FileCheck -check-prefixes=GFX9,GFX9-O0 /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/test/CodeGen/AMDGPU/wwm-reserved.ll
: 'RUN: at line 2';   /b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/bin/llc -march=amdgcn -mcpu=gfx900 -amdgpu-dpp-combine=false -verify-machineinstrs < /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/test/CodeGen/AMDGPU/wwm-reserved.ll | /b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/bin/FileCheck -check-prefixes=GFX9,GFX9-O3 /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/test/CodeGen/AMDGPU/wwm-reserved.ll
--
Exit Code: 2
Command Output (stderr):
--
=================================================================
==28739==ERROR: AddressSanitizer: use-after-poison on address 0x621000278298 at pc 0x000005ad3340 bp 0x7fff5c6564f0 sp 0x7fff5c6564e8
READ of size 8 at 0x621000278298 thread T0
    #0 0x5ad333f in getOpcode /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/CodeGen/MachineInstr.h:478:39
    #1 0x5ad333f in isRegSequence /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/CodeGen/MachineInstr.h:1272:12
    #2 0x5ad333f in (anonymous namespace)::SIFoldOperands::runOnMachineFunction(llvm::MachineFunction&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:1807:39
    #3 0x91e1d53 in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:72:13
    #4 0x9fbe63c in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1439:27
    #5 0x847fa3b in RunPassOnSCC /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:178:25
    #6 0x847fa3b in RunAllPassesOnSCC /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:476:9
    #7 0x847fa3b in (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:541:18
    #8 0x9fc08b8 in runOnModule /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1554:27
    #9 0x9fc08b8 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:542:44
    #10 0x4b13085 in compileModule /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/tools/llc/llc.cpp:687:8
    #11 0x4b13085 in main /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/tools/llc/llc.cpp:388:22
    #12 0x7f164c90a09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #13 0x4a336e9 in _start (/b/sanitizer-x86_64-linux-bootstrap/build/llvm_build_asan/bin/llc+0x4a336e9)
0x621000278298 is located 3480 bytes inside of 4096-byte region [0x621000277500,0x621000278500)
allocated by thread T0 here:
    #0 0x4b09ee8 in operator new(unsigned long) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:99:3
    #1 0x4f7d4b6 in Allocate /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/Support/AllocatorBase.h:85:12
    #2 0x4f7d4b6 in llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>::StartNewSlab() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/Support/Allocator.h:335:21
    #3 0x4f7d1d2 in llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>::Allocate(unsigned long, llvm::Align) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/Support/Allocator.h:190:5
    #4 0x91baad7 in Allocate /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/Support/Allocator.h:204:12
    #5 0x91baad7 in operator new<llvm::MallocAllocator, 4096, 4096, 128> /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/Support/Allocator.h:437:20
    #6 0x91baad7 in llvm::MachineFunction::init() /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/CodeGen/MachineFunction.cpp:160:15
    #7 0x9279d61 in llvm::MachineModuleInfo::getOrCreateMachineFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/CodeGen/MachineModuleInfo.cpp:296:14
    #8 0x91e1b8b in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:44:29
    #9 0x9fbe63c in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1439:27
    #10 0x847fa3b in RunPassOnSCC /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:178:25
    #11 0x847fa3b in RunAllPassesOnSCC /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:476:9
    #12 0x847fa3b in (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:541:18
    #13 0x9fc08b8 in runOnModule /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1554:27
    #14 0x9fc08b8 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:542:44
    #15 0x4b13085 in compileModule /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/tools/llc/llc.cpp:687:8
    #16 0x4b13085 in main /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/tools/llc/llc.cpp:388:22
    #17 0x7f164c90a09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
SUMMARY: AddressSanitizer: use-after-poison /b/sanitizer-x86_64-linux-bootstrap/build/llvm-project/llvm/include/llvm/CodeGen/MachineInstr.h:478:39 in getOpcode
Shadow bytes around the buggy address:
  0x0c4280047000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f7
  0x0c4280047010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4280047020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4280047030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4280047040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c4280047050: f7 f7 f7[f7]f7 f7 f7 f7 f7 f7 f7 00 00 00 00 00
  0x0c4280047060: 00 00 00 00 f7 00 00 00 00 00 00 00 00 00 f7 00
  0x0c4280047070: 00 00 00 00 00 00 00 f7 00 00 00 00 00 00 00 00
  0x0c4280047080: 00 f7 00 00 00 00 00 00 00 00 f7 00 00 00 00 00
  0x0c4280047090: 00 00 00 00 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c42800470a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==28739==ABORTING
hctim added a reverting change: rG3d4730a73fd1: Revert "[AMDGPU] SIFoldOperands: eagerly erase dead REG_SEQUENCEs".Apr 9 2021, 3:48 PM
foad added a commit: rG323ef0eb4590: [AMDGPU] SIFoldOperands: eagerly erase dead REG_SEQUENCEs.Apr 19 2021, 4:07 AM
foad mentioned this in rGef443390a97e: [AMDGPU] Remove MachineDCE after SIFoldOperands.Apr 19 2021, 4:12 AM

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
23 lines

Diff 336545

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

Show First 20 Lines • Show All 1,538 Lines • ▼ Show 20 Lines for (auto &Def : Defs) {
if (!SubDef || !SubDef->isCopy() || SubDef->getOperand(1).getSubReg()) if (!SubDef || !SubDef->isCopy() || SubDef->getOperand(1).getSubReg())
return false; return false;
if (!TRI->isAGPR(*MRI, SubDef->getOperand(1).getReg())) if (!TRI->isAGPR(*MRI, SubDef->getOperand(1).getReg()))
return false; return false;
} }
MachineOperand *Op = &*MRI->use_nodbg_begin(Reg); MachineOperand *Op = &*MRI->use_nodbg_begin(Reg);
MachineInstr *UseMI = Op->getParent(); MachineInstr *UseMI = Op->getParent();
while (UseMI->isCopy() && !Op->getSubReg()) { while (UseMI->isCopy() && !Op->getSubReg()) {
foadAuthorUnsubmitted
Done
ReplyInline Actions

Do we actually need to follow copies here? I don't see any lit tests regressions if I remove this loop.

foad: Do we actually need to follow copies here? I don't see any lit tests regressions if I remove…
rampitecUnsubmitted
Not Done
ReplyInline Actions

The relevant test was in the acc-ldst.ll at least initially, all the subtests with loops. It was written to fold a store from LCSSA PHI. We had too many changes since then though so the test probably lost some of its value, but the code is still relevant.

rampitec: The relevant test was in the acc-ldst.ll at least initially, all the subtests with loops. It…
Reg = UseMI->getOperand(0).getReg(); Reg = UseMI->getOperand(0).getReg();
if (!TRI->isVGPR(*MRI, Reg) || !MRI->hasOneNonDBGUse(Reg)) if (!TRI->isVGPR(*MRI, Reg) || !MRI->hasOneNonDBGUse(Reg))
return false; return false;
Op = &*MRI->use_nodbg_begin(Reg); Op = &*MRI->use_nodbg_begin(Reg);
UseMI = Op->getParent(); UseMI = Op->getParent();
} }
if (Op->getSubReg()) if (Op->getSubReg())
Show All 35 Linesbool SIFoldOperands::tryFoldRegSequence(MachineInstr &MI) {
if (!TII->isOperandLegal(*UseMI, OpIdx, Op)) { if (!TII->isOperandLegal(*UseMI, OpIdx, Op)) {
Op->setReg(Reg); Op->setReg(Reg);
RS->eraseFromParent(); RS->eraseFromParent();
return false; return false;
} }
LLVM_DEBUG(dbgs() << "Folded " << *RS << " into " << *UseMI); LLVM_DEBUG(dbgs() << "Folded " << *RS << " into " << *UseMI);
// Erase the REG_SEQUENCE eagerly, unless we followed a chain of COPY users,
// in which case we can erase them all later in runOnMachineFunction.
if (MRI->use_nodbg_empty(MI.getOperand(0).getReg()))
MI.eraseFromParentAndMarkDBGValuesForRemoval();
return true; return true;
} }
// Try to hoist an AGPR to VGPR copy out of the loop across a LCSSA PHI. // Try to hoist an AGPR to VGPR copy out of the loop across a LCSSA PHI.
// This should allow folding of an AGPR into a consumer which may support it. // This should allow folding of an AGPR into a consumer which may support it.
// I.e.: // I.e.:
// //
// loop: // loop: // loop: // loop:
▲ Show 20 Lines • Show All 174 Lines • ▼ Show 20 Lines for (auto &MI : make_early_inc_range(*MBB)) {
// %vgpr0 = V_MOV_B32_e32 1, implicit %exec // %vgpr0 = V_MOV_B32_e32 1, implicit %exec
if (!MI.getOperand(0).getReg().isVirtual()) if (!MI.getOperand(0).getReg().isVirtual())
continue; continue;
foldInstOperand(MI, OpToFold); foldInstOperand(MI, OpToFold);
// If we managed to fold all uses of this copy then we might as well // If we managed to fold all uses of this copy then we might as well
// delete it now. // delete it now.
if (MRI->use_nodbg_empty(MI.getOperand(0).getReg())) // The only reason we need to follow chains of copies here is that
MI.eraseFromParentAndMarkDBGValuesForRemoval(); // tryFoldRegSequence looks forward through copies before folding a
// REG_SEQUENCE into its eventual users.
auto *InstToErase = &MI;
while (MRI->use_nodbg_empty(InstToErase->getOperand(0).getReg())) {
auto &SrcOp = InstToErase->getOperand(1);
auto SrcReg = SrcOp.isReg() ? SrcOp.getReg() : Register();
InstToErase->eraseFromParentAndMarkDBGValuesForRemoval();
if (!SrcReg || SrcReg.isPhysical())
break;
InstToErase = MRI->getVRegDef(SrcReg);
if (!InstToErase || !TII->isFoldableCopy(*InstToErase))
break;
}
if (InstToErase && InstToErase->isRegSequence() &&
MRI->use_nodbg_empty(InstToErase->getOperand(0).getReg()))
InstToErase->eraseFromParentAndMarkDBGValuesForRemoval();
} }
} }
return true; return true;
} }