[llvm] 187686a - [GlobalISel] LegalizationArtifactCombiner: Fix a bug in tryCombineMerges
Volkan Keles via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 14 10:46:15 PST 2020
Author: Volkan Keles
Date: 2020-02-14T10:45:58-08:00
New Revision: 187686a22f97662c981b9bffb998b6010ae1c401
URL: https://github.com/llvm/llvm-project/commit/187686a22f97662c981b9bffb998b6010ae1c401
DIFF: https://github.com/llvm/llvm-project/commit/187686a22f97662c981b9bffb998b6010ae1c401.diff
LOG: [GlobalISel] LegalizationArtifactCombiner: Fix a bug in tryCombineMerges
Like COPY instructions explained in D70616, we don't check the constraints
when combining G_UNMERGE_VALUES. Use the same logic used in D70616 to check
if registers can be replaced, or a COPY instruction needs to be built.
https://reviews.llvm.org/D70564
Added:
llvm/test/CodeGen/AArch64/GlobalISel/artifact-combine-unmerge.mir
Modified:
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
llvm/lib/CodeGen/GlobalISel/Utils.cpp
llvm/lib/Target/Mips/MipsRegisterBankInfo.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
index b4f9b96653c5..af8129b98a2b 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
@@ -280,9 +280,36 @@ class LegalizationArtifactCombiner {
}
}
+ /// Try to replace DstReg with SrcReg or build a COPY instruction
+ /// depending on the register constraints.
+ static void replaceRegOrBuildCopy(Register DstReg, Register SrcReg,
+ MachineRegisterInfo &MRI,
+ MachineIRBuilder &Builder,
+ SmallVectorImpl<Register> &UpdatedDefs,
+ GISelObserverWrapper &Observer) {
+ if (!llvm::canReplaceReg(DstReg, SrcReg, MRI)) {
+ Builder.buildCopy(DstReg, SrcReg);
+ UpdatedDefs.push_back(DstReg);
+ return;
+ }
+ SmallVector<MachineInstr *, 4> UseMIs;
+ // Get the users and notify the observer before replacing.
+ for (auto &UseMI : MRI.use_instructions(DstReg)) {
+ UseMIs.push_back(&UseMI);
+ Observer.changingInstr(UseMI);
+ }
+ // Replace the registers.
+ MRI.replaceRegWith(DstReg, SrcReg);
+ UpdatedDefs.push_back(SrcReg);
+ // Notify the observer that we changed the instructions.
+ for (auto *UseMI : UseMIs)
+ Observer.changedInstr(*UseMI);
+ }
+
bool tryCombineMerges(MachineInstr &MI,
SmallVectorImpl<MachineInstr *> &DeadInsts,
- SmallVectorImpl<Register> &UpdatedDefs) {
+ SmallVectorImpl<Register> &UpdatedDefs,
+ GISelObserverWrapper &Observer) {
assert(MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES);
unsigned NumDefs = MI.getNumOperands() - 1;
@@ -395,10 +422,12 @@ class LegalizationArtifactCombiner {
"Bitcast and the other kinds of conversions should "
"have happened earlier");
+ Builder.setInstr(MI);
for (unsigned Idx = 0; Idx < NumDefs; ++Idx) {
- Register NewDef = MergeI->getOperand(Idx + 1).getReg();
- MRI.replaceRegWith(MI.getOperand(Idx).getReg(), NewDef);
- UpdatedDefs.push_back(NewDef);
+ Register DstReg = MI.getOperand(Idx).getReg();
+ Register SrcReg = MergeI->getOperand(Idx + 1).getReg();
+ replaceRegOrBuildCopy(DstReg, SrcReg, MRI, Builder, UpdatedDefs,
+ Observer);
}
}
@@ -498,7 +527,7 @@ class LegalizationArtifactCombiner {
Changed = tryCombineSExt(MI, DeadInsts, UpdatedDefs);
break;
case TargetOpcode::G_UNMERGE_VALUES:
- Changed = tryCombineMerges(MI, DeadInsts, UpdatedDefs);
+ Changed = tryCombineMerges(MI, DeadInsts, UpdatedDefs, WrapperObserver);
break;
case TargetOpcode::G_EXTRACT:
Changed = tryCombineExtract(MI, DeadInsts, UpdatedDefs);
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/Utils.h b/llvm/include/llvm/CodeGen/GlobalISel/Utils.h
index 63c5746bf183..a88a97c666ad 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/Utils.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/Utils.h
@@ -93,6 +93,11 @@ bool constrainSelectedInstRegOperands(MachineInstr &I,
const TargetInstrInfo &TII,
const TargetRegisterInfo &TRI,
const RegisterBankInfo &RBI);
+
+/// Check if DstReg can be replaced with SrcReg depending on the register
+/// constraints.
+bool canReplaceReg(Register DstReg, Register SrcReg, MachineRegisterInfo &MRI);
+
/// Check whether an instruction \p MI is dead: it only defines dead virtual
/// registers, and doesn't have other side effects.
bool isTriviallyDead(const MachineInstr &MI, const MachineRegisterInfo &MRI);
diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index cc8f5a10ca07..79fbe1db9d3f 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -75,36 +75,7 @@ bool CombinerHelper::matchCombineCopy(MachineInstr &MI) {
return false;
Register DstReg = MI.getOperand(0).getReg();
Register SrcReg = MI.getOperand(1).getReg();
-
- // Give up if either DstReg or SrcReg is a physical register.
- if (Register::isPhysicalRegister(DstReg) ||
- Register::isPhysicalRegister(SrcReg))
- return false;
-
- // Give up the types don't match.
- LLT DstTy = MRI.getType(DstReg);
- LLT SrcTy = MRI.getType(SrcReg);
- // Give up if one has a valid LLT, but the other doesn't.
- if (DstTy.isValid() != SrcTy.isValid())
- return false;
- // Give up if the types don't match.
- if (DstTy.isValid() && SrcTy.isValid() && DstTy != SrcTy)
- return false;
-
- // Get the register banks and classes.
- const RegisterBank *DstBank = MRI.getRegBankOrNull(DstReg);
- const RegisterBank *SrcBank = MRI.getRegBankOrNull(SrcReg);
- const TargetRegisterClass *DstRC = MRI.getRegClassOrNull(DstReg);
- const TargetRegisterClass *SrcRC = MRI.getRegClassOrNull(SrcReg);
-
- // Replace if the register constraints match.
- if ((SrcRC == DstRC) && (SrcBank == DstBank))
- return true;
- // Replace if DstReg has no constraints.
- if (!DstBank && !DstRC)
- return true;
-
- return false;
+ return canReplaceReg(DstReg, SrcReg, MRI);
}
void CombinerHelper::applyCombineCopy(MachineInstr &MI) {
Register DstReg = MI.getOperand(0).getReg();
diff --git a/llvm/lib/CodeGen/GlobalISel/Utils.cpp b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
index 5f72974b31ec..d29e9546be0b 100644
--- a/llvm/lib/CodeGen/GlobalISel/Utils.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
@@ -155,6 +155,20 @@ bool llvm::constrainSelectedInstRegOperands(MachineInstr &I,
return true;
}
+bool llvm::canReplaceReg(Register DstReg, Register SrcReg,
+ MachineRegisterInfo &MRI) {
+ // Give up if either DstReg or SrcReg is a physical register.
+ if (DstReg.isPhysical() || SrcReg.isPhysical())
+ return false;
+ // Give up if the types don't match.
+ if (MRI.getType(DstReg) != MRI.getType(SrcReg))
+ return false;
+ // Replace if either DstReg has no constraints or the register
+ // constraints match.
+ return !MRI.getRegClassOrRegBank(DstReg) ||
+ MRI.getRegClassOrRegBank(DstReg) == MRI.getRegClassOrRegBank(SrcReg);
+}
+
bool llvm::isTriviallyDead(const MachineInstr &MI,
const MachineRegisterInfo &MRI) {
// If we can move an instruction, we can remove it. Otherwise, it has
diff --git a/llvm/lib/Target/Mips/MipsRegisterBankInfo.cpp b/llvm/lib/Target/Mips/MipsRegisterBankInfo.cpp
index 225cfa0cc4ef..754cc9406269 100644
--- a/llvm/lib/Target/Mips/MipsRegisterBankInfo.cpp
+++ b/llvm/lib/Target/Mips/MipsRegisterBankInfo.cpp
@@ -653,10 +653,10 @@ void MipsRegisterBankInfo::setRegBank(MachineInstr &MI,
static void
combineAwayG_UNMERGE_VALUES(LegalizationArtifactCombiner &ArtCombiner,
- MachineInstr &MI) {
+ MachineInstr &MI, GISelObserverWrapper &Observer) {
SmallVector<Register, 4> UpdatedDefs;
SmallVector<MachineInstr *, 2> DeadInstrs;
- ArtCombiner.tryCombineMerges(MI, DeadInstrs, UpdatedDefs);
+ ArtCombiner.tryCombineMerges(MI, DeadInstrs, UpdatedDefs, Observer);
for (MachineInstr *DeadMI : DeadInstrs)
DeadMI->eraseFromParent();
}
@@ -689,7 +689,7 @@ void MipsRegisterBankInfo::applyMappingImpl(
// not be considered for regbank selection. RegBankSelect for mips
// visits/makes corresponding G_MERGE first. Combine them here.
if (NewMI->getOpcode() == TargetOpcode::G_UNMERGE_VALUES)
- combineAwayG_UNMERGE_VALUES(ArtCombiner, *NewMI);
+ combineAwayG_UNMERGE_VALUES(ArtCombiner, *NewMI, WrapperObserver);
// This G_MERGE will be combined away when its corresponding G_UNMERGE
// gets regBankSelected.
else if (NewMI->getOpcode() == TargetOpcode::G_MERGE_VALUES)
@@ -701,7 +701,7 @@ void MipsRegisterBankInfo::applyMappingImpl(
return;
}
case TargetOpcode::G_UNMERGE_VALUES:
- combineAwayG_UNMERGE_VALUES(ArtCombiner, MI);
+ combineAwayG_UNMERGE_VALUES(ArtCombiner, MI, WrapperObserver);
return;
default:
break;
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/artifact-combine-unmerge.mir b/llvm/test/CodeGen/AArch64/GlobalISel/artifact-combine-unmerge.mir
new file mode 100644
index 000000000000..3e42fd5b99e3
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/artifact-combine-unmerge.mir
@@ -0,0 +1,73 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -o - -march=aarch64 -run-pass=legalizer %s | FileCheck %s
+
+# Make sure we don't lose the register bank constraints when
+# artifact combining G_UNMERGE_VALUES instructions.
+---
+name: test_none_none
+body: |
+ bb.0.entry:
+ ; CHECK-LABEL: name: test_none_none
+ ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $w0
+ ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $w1
+ ; CHECK: $w0 = COPY [[COPY]](s32)
+ ; CHECK: $w1 = COPY [[COPY1]](s32)
+ %0:_(s32) = COPY $w0
+ %1:_(s32) = COPY $w1
+ %2:_(s64) = G_MERGE_VALUES %0(s32), %1
+ %3:_(s32), %4:_(s32) = G_UNMERGE_VALUES %2(s64)
+ $w0 = COPY %3(s32)
+ $w1 = COPY %4(s32)
+...
+---
+name: test_gpr_none
+body: |
+ bb.0.entry:
+ ; CHECK-LABEL: name: test_gpr_none
+ ; CHECK: [[COPY:%[0-9]+]]:gpr(s32) = COPY $w0
+ ; CHECK: [[COPY1:%[0-9]+]]:gpr(s32) = COPY $w1
+ ; CHECK: $w0 = COPY [[COPY]](s32)
+ ; CHECK: $w1 = COPY [[COPY1]](s32)
+ %0:gpr(s32) = COPY $w0
+ %1:gpr(s32) = COPY $w1
+ %2:_(s64) = G_MERGE_VALUES %0(s32), %1
+ %3:_(s32), %4:_(s32) = G_UNMERGE_VALUES %2(s64)
+ $w0 = COPY %3(s32)
+ $w1 = COPY %4(s32)
+...
+---
+name: test_none_gpr
+body: |
+ bb.0.entry:
+ ; CHECK-LABEL: name: test_none_gpr
+ ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $w0
+ ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $w1
+ ; CHECK: [[COPY2:%[0-9]+]]:gpr(s32) = COPY [[COPY]](s32)
+ ; CHECK: [[COPY3:%[0-9]+]]:gpr(s32) = COPY [[COPY1]](s32)
+ ; CHECK: $w0 = COPY [[COPY2]](s32)
+ ; CHECK: $w1 = COPY [[COPY3]](s32)
+ %0:_(s32) = COPY $w0
+ %1:_(s32) = COPY $w1
+ %2:_(s64) = G_MERGE_VALUES %0(s32), %1
+ %3:gpr(s32), %4:gpr(s32) = G_UNMERGE_VALUES %2(s64)
+ $w0 = COPY %3(s32)
+ $w1 = COPY %4(s32)
+...
+---
+name: test_fpr_gpr
+body: |
+ bb.0.entry:
+ ; CHECK-LABEL: name: test_fpr_gpr
+ ; CHECK: [[COPY:%[0-9]+]]:fpr(s32) = COPY $w0
+ ; CHECK: [[COPY1:%[0-9]+]]:fpr(s32) = COPY $w1
+ ; CHECK: [[COPY2:%[0-9]+]]:gpr(s32) = COPY [[COPY]](s32)
+ ; CHECK: [[COPY3:%[0-9]+]]:gpr(s32) = COPY [[COPY1]](s32)
+ ; CHECK: $w0 = COPY [[COPY2]](s32)
+ ; CHECK: $w1 = COPY [[COPY3]](s32)
+ %0:fpr(s32) = COPY $w0
+ %1:fpr(s32) = COPY $w1
+ %2:_(s64) = G_MERGE_VALUES %0(s32), %1
+ %3:gpr(s32), %4:gpr(s32) = G_UNMERGE_VALUES %2(s64)
+ $w0 = COPY %3(s32)
+ $w1 = COPY %4(s32)
+...
More information about the llvm-commits
mailing list