Skip to content

Commit

Permalink
[AMDGPU] Update LiveVariables in convertToThreeAddress()
Browse files Browse the repository at this point in the history 
This can fix an asan failure like below.
==15856==ERROR: AddressSanitizer: use-after-poison on address ...
READ of size 8 at 0x6210001a3cb0 thread T0
    #0 llvm::MachineInstr::getParent()
    #1 llvm::LiveVariables::VarInfo::findKill()
    #2 TwoAddressInstructionPass::rescheduleMIBelowKill()
    #3 TwoAddressInstructionPass::tryInstructionTransform()
    #4 TwoAddressInstructionPass::runOnMachineFunction()

We need to update the Kills if we replace instructions. The Kills
may be later accessed within TwoAddressInstruction pass.

Differential Revision: https://reviews.llvm.org/D89092
  • Loading branch information
@ruiling
ruiling committed Oct 13, 2020
1 parent 7f8dc34 commit b215a26
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 38 deletions.
101 changes: 63 additions & 38 deletions llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
#include "AMDGPU.h"
#include "AMDGPUSubtarget.h"
#include "GCNHazardRecognizer.h"
#include "MCTargetDesc/AMDGPUMCTargetDesc.h"
#include "SIDefines.h"
#include "SIMachineFunctionInfo.h"
#include "SIRegisterInfo.h"
#include "MCTargetDesc/AMDGPUMCTargetDesc.h"
#include "Utils/AMDGPUBaseInfo.h"
#include "llvm/ADT/APInt.h"
#include "llvm/ADT/ArrayRef.h"
Expand All @@ -28,6 +28,7 @@
#include "llvm/Analysis/AliasAnalysis.h"
#include "llvm/Analysis/MemoryLocation.h"
#include "llvm/Analysis/ValueTracking.h"
#include "llvm/CodeGen/LiveVariables.h"
#include "llvm/CodeGen/MachineBasicBlock.h"
#include "llvm/CodeGen/MachineDominators.h"
#include "llvm/CodeGen/MachineFrameInfo.h"
Expand Down Expand Up @@ -2841,6 +2842,18 @@ static int64_t getFoldableImm(const MachineOperand* MO) {
return AMDGPU::NoRegister;
}

static void updateLiveVariables(LiveVariables *LV, MachineInstr &MI,
MachineInstr &NewMI) {
if (LV) {
unsigned NumOps = MI.getNumOperands();
for (unsigned I = 1; I < NumOps; ++I) {
MachineOperand &Op = MI.getOperand(I);
if (Op.isReg() && Op.isKill())
LV->replaceKillInstruction(Op.getReg(), MI, NewMI);
}
}
}

MachineInstr *SIInstrInfo::convertToThreeAddress(MachineFunction::iterator &MBB,
MachineInstr &MI,
LiveVariables *LV) const {
Expand Down Expand Up @@ -2888,43 +2901,53 @@ MachineInstr *SIInstrInfo::convertToThreeAddress(MachineFunction::iterator &MBB,
const MachineOperand *Src2 = getNamedOperand(MI, AMDGPU::OpName::src2);
const MachineOperand *Clamp = getNamedOperand(MI, AMDGPU::OpName::clamp);
const MachineOperand *Omod = getNamedOperand(MI, AMDGPU::OpName::omod);
MachineInstrBuilder MIB;

if (!Src0Mods && !Src1Mods && !Clamp && !Omod &&
// If we have an SGPR input, we will violate the constant bus restriction.
(ST.getConstantBusLimit(Opc) > 1 ||
!Src0->isReg() ||
(ST.getConstantBusLimit(Opc) > 1 || !Src0->isReg() ||
!RI.isSGPRReg(MBB->getParent()->getRegInfo(), Src0->getReg()))) {
if (auto Imm = getFoldableImm(Src2)) {
unsigned NewOpc =
IsFMA ? (IsF16 ? AMDGPU::V_FMAAK_F16 : AMDGPU::V_FMAAK_F32)
: (IsF16 ? AMDGPU::V_MADAK_F16 : AMDGPU::V_MADAK_F32);
if (pseudoToMCOpcode(NewOpc) != -1)
return BuildMI(*MBB, MI, MI.getDebugLoc(), get(NewOpc))
.add(*Dst)
.add(*Src0)
.add(*Src1)
.addImm(Imm);
}
unsigned NewOpc =
IsFMA ? (IsF16 ? AMDGPU::V_FMAMK_F16 : AMDGPU::V_FMAMK_F32)
: (IsF16 ? AMDGPU::V_MADMK_F16 : AMDGPU::V_MADMK_F32);
IsFMA ? (IsF16 ? AMDGPU::V_FMAAK_F16 : AMDGPU::V_FMAAK_F32)
: (IsF16 ? AMDGPU::V_MADAK_F16 : AMDGPU::V_MADAK_F32);
if (pseudoToMCOpcode(NewOpc) != -1) {
MIB = BuildMI(*MBB, MI, MI.getDebugLoc(), get(NewOpc))
.add(*Dst)
.add(*Src0)
.add(*Src1)
.addImm(Imm);
updateLiveVariables(LV, MI, *MIB);
return MIB;
}
}
unsigned NewOpc = IsFMA
? (IsF16 ? AMDGPU::V_FMAMK_F16 : AMDGPU::V_FMAMK_F32)
: (IsF16 ? AMDGPU::V_MADMK_F16 : AMDGPU::V_MADMK_F32);
if (auto Imm = getFoldableImm(Src1)) {
if (pseudoToMCOpcode(NewOpc) != -1)
return BuildMI(*MBB, MI, MI.getDebugLoc(), get(NewOpc))
.add(*Dst)
.add(*Src0)
.addImm(Imm)
.add(*Src2);
if (pseudoToMCOpcode(NewOpc) != -1) {
MIB = BuildMI(*MBB, MI, MI.getDebugLoc(), get(NewOpc))
.add(*Dst)
.add(*Src0)
.addImm(Imm)
.add(*Src2);
updateLiveVariables(LV, MI, *MIB);
return MIB;
}
}
if (auto Imm = getFoldableImm(Src0)) {
if (pseudoToMCOpcode(NewOpc) != -1 &&
isOperandLegal(MI, AMDGPU::getNamedOperandIdx(NewOpc,
AMDGPU::OpName::src0), Src1))
return BuildMI(*MBB, MI, MI.getDebugLoc(), get(NewOpc))
.add(*Dst)
.add(*Src1)
.addImm(Imm)
.add(*Src2);
isOperandLegal(
MI, AMDGPU::getNamedOperandIdx(NewOpc, AMDGPU::OpName::src0),
Src1)) {
MIB = BuildMI(*MBB, MI, MI.getDebugLoc(), get(NewOpc))
.add(*Dst)
.add(*Src1)
.addImm(Imm)
.add(*Src2);
updateLiveVariables(LV, MI, *MIB);
return MIB;
}
}
}

Expand All @@ -2933,16 +2956,18 @@ MachineInstr *SIInstrInfo::convertToThreeAddress(MachineFunction::iterator &MBB,
if (pseudoToMCOpcode(NewOpc) == -1)
return nullptr;

return BuildMI(*MBB, MI, MI.getDebugLoc(), get(NewOpc))
.add(*Dst)
.addImm(Src0Mods ? Src0Mods->getImm() : 0)
.add(*Src0)
.addImm(Src1Mods ? Src1Mods->getImm() : 0)
.add(*Src1)
.addImm(0) // Src mods
.add(*Src2)
.addImm(Clamp ? Clamp->getImm() : 0)
.addImm(Omod ? Omod->getImm() : 0);
MIB = BuildMI(*MBB, MI, MI.getDebugLoc(), get(NewOpc))
.add(*Dst)
.addImm(Src0Mods ? Src0Mods->getImm() : 0)
.add(*Src0)
.addImm(Src1Mods ? Src1Mods->getImm() : 0)
.add(*Src1)
.addImm(0) // Src mods
.add(*Src2)
.addImm(Clamp ? Clamp->getImm() : 0)
.addImm(Omod ? Omod->getImm() : 0);
updateLiveVariables(LV, MI, *MIB);
return MIB;
}

// It's not generally safe to move VALU instructions across these since it will
Expand Down
36 changes: 36 additions & 0 deletions llvm/test/CodeGen/AMDGPU/stale-livevar-in-twoaddr-pass.mir
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# RUN: llc -march=amdgcn -mcpu=gfx900 -run-pass=livevars,phi-node-elimination,twoaddressinstruction -verify-machineinstrs -o - %s | FileCheck %s
# This used to fail under ASAN enabled build because we didn't update LiveVariables in SIInstrInfo::convertToThreeAddress()
# CHECK: _amdgpu_ps_main

---
name: _amdgpu_ps_main
alignment: 1
tracksRegLiveness: true
body: |
bb.0:
liveins: $sgpr2, $vgpr2, $vgpr3
%0:vgpr_32 = COPY $vgpr3
%1:vgpr_32 = COPY $vgpr2
S_BRANCH %bb.3
bb.1:
%2:vgpr_32 = V_MAC_F32_e32 0, %0, %1, implicit $mode, implicit $exec
%3:vgpr_32 = V_MED3_F32 0, %1, 0, %2, 0, %2, 0, 0, implicit $mode, implicit $exec
bb.2:
%4:vgpr_32 = PHI %5, %bb.3, %3, %bb.1
SI_END_CF %6, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
EXP_DONE 0, %4, %4, %4, %4, -1, 0, 15, implicit $exec
S_ENDPGM 0
bb.3:
successors: %bb.1, %bb.2
%5:vgpr_32 = V_MAC_F32_e32 0, %1, %0, implicit $mode, implicit $exec
%7:vgpr_32 = V_CVT_I32_F32_e32 %5, implicit $mode, implicit $exec
%8:sreg_64 = V_CMP_NE_U32_e64 1, %7, implicit $exec
%6:sreg_64 = SI_IF %8, %bb.2, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
S_BRANCH %bb.1
...

0 comments on commit b215a26

Please sign in to comment.