GlobalISel/MachineIRBuilder: Construct DstOp with VRegAttrs #113581
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-backend-amdgpu Author: Petar Avramovic (petar-avramovic) ChangesAllow construction of DstOp with VRegAttrs. Full diff: https://github.com/llvm/llvm-project/pull/113581.diff 7 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h b/llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
index 816e94362f0262..8ce6eaa69c4ab7 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
@@ -17,6 +17,7 @@
#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
#include "llvm/CodeGen/GlobalISel/GISelWorkList.h"
#include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/CodeGen/MachineRegisterInfo.h"
#include "llvm/Support/Allocator.h"
#include "llvm/Support/CodeGen.h"
@@ -177,6 +178,8 @@ class GISelInstProfileBuilder {
const GISelInstProfileBuilder &addNodeIDOpcode(unsigned Opc) const;
const GISelInstProfileBuilder &addNodeIDRegType(const LLT Ty) const;
const GISelInstProfileBuilder &addNodeIDRegType(const Register) const;
+ const GISelInstProfileBuilder &
+ addNodeIDRegType(MachineRegisterInfo::VRegAttrs) const;
const GISelInstProfileBuilder &
addNodeIDRegType(const TargetRegisterClass *RC) const;
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index 9b993482c8cc07..d17016544dafb1 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -72,15 +72,20 @@ class DstOp {
LLT LLTTy;
Register Reg;
const TargetRegisterClass *RC;
+ MachineRegisterInfo::VRegAttrs Attrs;
};
public:
- enum class DstType { Ty_LLT, Ty_Reg, Ty_RC };
+ enum class DstType { Ty_LLT, Ty_Reg, Ty_RC, Ty_VRegAttrs };
DstOp(unsigned R) : Reg(R), Ty(DstType::Ty_Reg) {}
DstOp(Register R) : Reg(R), Ty(DstType::Ty_Reg) {}
DstOp(const MachineOperand &Op) : Reg(Op.getReg()), Ty(DstType::Ty_Reg) {}
DstOp(const LLT T) : LLTTy(T), Ty(DstType::Ty_LLT) {}
DstOp(const TargetRegisterClass *TRC) : RC(TRC), Ty(DstType::Ty_RC) {}
+ DstOp(MachineRegisterInfo::VRegAttrs Attrs)
+ : Attrs(Attrs), Ty(DstType::Ty_VRegAttrs) {}
+ DstOp(RegClassOrRegBank RCOrRB, LLT Ty)
+ : Attrs({RCOrRB, Ty}), Ty(DstType::Ty_VRegAttrs) {}
void addDefToMIB(MachineRegisterInfo &MRI, MachineInstrBuilder &MIB) const {
switch (Ty) {
@@ -93,6 +98,9 @@ class DstOp {
case DstType::Ty_RC:
MIB.addDef(MRI.createVirtualRegister(RC));
break;
+ case DstType::Ty_VRegAttrs:
+ MIB.addDef(MRI.createVirtualRegister(Attrs));
+ break;
}
}
@@ -104,6 +112,8 @@ class DstOp {
return LLTTy;
case DstType::Ty_Reg:
return MRI.getType(Reg);
+ case DstType::Ty_VRegAttrs:
+ return Attrs.Ty;
}
llvm_unreachable("Unrecognised DstOp::DstType enum");
}
@@ -122,6 +132,15 @@ class DstOp {
}
}
+ MachineRegisterInfo::VRegAttrs getVRegAttrs() const {
+ switch (Ty) {
+ case DstType::Ty_VRegAttrs:
+ return Attrs;
+ default:
+ llvm_unreachable("Not a VRegAttrs Operand");
+ }
+ }
+
DstType getDstOpKind() const { return Ty; }
private:
diff --git a/llvm/include/llvm/CodeGen/MachineRegisterInfo.h b/llvm/include/llvm/CodeGen/MachineRegisterInfo.h
index 7a2c23c13a3ce6..5dc51aaed81c7b 100644
--- a/llvm/include/llvm/CodeGen/MachineRegisterInfo.h
+++ b/llvm/include/llvm/CodeGen/MachineRegisterInfo.h
@@ -754,7 +754,7 @@ class MachineRegisterInfo {
/// Returns register class or bank and low level type of \p Reg. Always safe
/// to use. Special values are returned when \p Reg does not have some of the
/// attributes.
- VRegAttrs getVRegAttrs(Register Reg) {
+ VRegAttrs getVRegAttrs(Register Reg) const {
return {getRegClassOrRegBank(Reg), getType(Reg)};
}
diff --git a/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp b/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
index ca4d0986b4426c..d3d545dab7dbc4 100644
--- a/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
@@ -356,6 +356,23 @@ GISelInstProfileBuilder::addNodeIDRegType(const RegisterBank *RB) const {
return *this;
}
+const GISelInstProfileBuilder &GISelInstProfileBuilder::addNodeIDRegType(
+ MachineRegisterInfo::VRegAttrs Attrs) const {
+ LLT Ty = Attrs.Ty;
+ if (Ty.isValid())
+ addNodeIDRegType(Ty);
+
+ const RegClassOrRegBank &RCOrRB = Attrs.RCOrRB;
+ if (RCOrRB) {
+ if (const auto *RB = dyn_cast_if_present<const RegisterBank *>(RCOrRB))
+ addNodeIDRegType(RB);
+ else if (const auto *RC =
+ dyn_cast_if_present<const TargetRegisterClass *>(RCOrRB))
+ addNodeIDRegType(RC);
+ }
+ return *this;
+}
+
const GISelInstProfileBuilder &
GISelInstProfileBuilder::addNodeIDImmediate(int64_t Imm) const {
ID.AddInteger(Imm);
@@ -389,17 +406,7 @@ GISelInstProfileBuilder::addNodeIDFlag(unsigned Flag) const {
const GISelInstProfileBuilder &
GISelInstProfileBuilder::addNodeIDReg(Register Reg) const {
- LLT Ty = MRI.getType(Reg);
- if (Ty.isValid())
- addNodeIDRegType(Ty);
-
- if (const RegClassOrRegBank &RCOrRB = MRI.getRegClassOrRegBank(Reg)) {
- if (const auto *RB = dyn_cast_if_present<const RegisterBank *>(RCOrRB))
- addNodeIDRegType(RB);
- else if (const auto *RC =
- dyn_cast_if_present<const TargetRegisterClass *>(RCOrRB))
- addNodeIDRegType(RC);
- }
+ addNodeIDRegType(MRI.getVRegAttrs(Reg));
return *this;
}
diff --git a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
index 547529bbe699ab..bf8e847011d7c1 100644
--- a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
@@ -73,18 +73,24 @@ bool CSEMIRBuilder::canPerformCSEForOpc(unsigned Opc) const {
void CSEMIRBuilder::profileDstOp(const DstOp &Op,
GISelInstProfileBuilder &B) const {
switch (Op.getDstOpKind()) {
- case DstOp::DstType::Ty_RC:
+ case DstOp::DstType::Ty_RC: {
B.addNodeIDRegType(Op.getRegClass());
break;
+ }
case DstOp::DstType::Ty_Reg: {
// Regs can have LLT&(RB|RC). If those exist, profile them as well.
B.addNodeIDReg(Op.getReg());
break;
}
- default:
+ case DstOp::DstType::Ty_LLT: {
B.addNodeIDRegType(Op.getLLTTy(*getMRI()));
break;
}
+ case DstOp::DstType::Ty_VRegAttrs: {
+ B.addNodeIDRegType(Op.getVRegAttrs());
+ break;
+ }
+ }
}
void CSEMIRBuilder::profileSrcOp(const SrcOp &Op,
diff --git a/llvm/unittests/Target/AMDGPU/CMakeLists.txt b/llvm/unittests/Target/AMDGPU/CMakeLists.txt
index e0efb967b5941d..ca8f48bc393efd 100644
--- a/llvm/unittests/Target/AMDGPU/CMakeLists.txt
+++ b/llvm/unittests/Target/AMDGPU/CMakeLists.txt
@@ -11,6 +11,7 @@ set(LLVM_LINK_COMPONENTS
CodeGen
CodeGenTypes
Core
+ GlobalISel
MC
Support
TargetParser
@@ -18,6 +19,7 @@ set(LLVM_LINK_COMPONENTS
add_llvm_target_unittest(AMDGPUTests
AMDGPUUnitTests.cpp
+ CSETest.cpp
DwarfRegMappings.cpp
ExecMayBeModifiedBeforeAnyUse.cpp
PALMetadata.cpp
diff --git a/llvm/unittests/Target/AMDGPU/CSETest.cpp b/llvm/unittests/Target/AMDGPU/CSETest.cpp
new file mode 100644
index 00000000000000..20ce2599e56aac
--- /dev/null
+++ b/llvm/unittests/Target/AMDGPU/CSETest.cpp
@@ -0,0 +1,75 @@
+//===- llvm/unittests/Target/AMDGPU/ExecMayBeModifiedBeforeAnyUse.cpp -----===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AMDGPUTargetMachine.h"
+#include "AMDGPUUnitTests.h"
+#include "llvm/CodeGen/GlobalISel/CSEInfo.h"
+#include "llvm/CodeGen/GlobalISel/CSEMIRBuilder.h"
+#include "llvm/CodeGen/MachineRegionInfo.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+TEST(AMDGPU, TestCSEForRegisterClassOrBankAndLLT) {
+ auto TM = createAMDGPUTargetMachine("amdgcn-amd-", "gfx1100", "");
+ if (!TM)
+ GTEST_SKIP();
+
+ GCNSubtarget ST(TM->getTargetTriple(), std::string(TM->getTargetCPU()),
+ std::string(TM->getTargetFeatureString()), *TM);
+
+ LLVMContext Ctx;
+ Module Mod("Module", Ctx);
+ Mod.setDataLayout(TM->createDataLayout());
+
+ auto *Type = FunctionType::get(Type::getVoidTy(Ctx), false);
+ auto *F = Function::Create(Type, GlobalValue::ExternalLinkage, "Test", &Mod);
+
+ MachineModuleInfo MMI(TM.get());
+ auto MF =
+ std::make_unique<MachineFunction>(*F, *TM, ST, MMI.getContext(), 42);
+ auto *BB = MF->CreateMachineBasicBlock();
+ MF->push_back(BB);
+
+ MachineIRBuilder B(*MF);
+ B.setMBB(*BB);
+
+ LLT S32{LLT::scalar(32)};
+ Register R0 = B.buildCopy(S32, Register(AMDGPU::SGPR0)).getReg(0);
+ Register R1 = B.buildCopy(S32, Register(AMDGPU::SGPR1)).getReg(0);
+
+ GISelCSEInfo CSEInfo;
+ CSEInfo.setCSEConfig(std::make_unique<CSEConfigFull>());
+ CSEInfo.analyze(*MF);
+ B.setCSEInfo(&CSEInfo);
+ CSEMIRBuilder CSEB(B.getState());
+ CSEB.setInsertPt(B.getMBB(), B.getInsertPt());
+
+ const RegisterBankInfo &RBI = *MF->getSubtarget().getRegBankInfo();
+
+ const TargetRegisterClass *SgprRC = &AMDGPU::SReg_32RegClass;
+ const RegisterBank *SgprRB = &RBI.getRegBank(AMDGPU::SGPRRegBankID);
+ MachineRegisterInfo::VRegAttrs SgprRCS32 = {SgprRC, S32};
+ MachineRegisterInfo::VRegAttrs SgprRBS32 = {SgprRB, S32};
+
+ auto AddLLT = CSEB.buildAdd(S32, R0, R1);
+ auto AddRCLLT = CSEB.buildInstr(AMDGPU::G_ADD, {SgprRCS32}, {R0, R1});
+ auto AddRBLLT = CSEB.buildInstr(AMDGPU::G_ADD, {{SgprRB, S32}}, {R0, R1});
+
+ ASSERT_NE(AddLLT, AddRCLLT);
+ ASSERT_NE(AddLLT, AddRBLLT);
+ ASSERT_NE(AddRCLLT, AddRBLLT);
+
+ auto AddLLT_CSE = CSEB.buildAdd(S32, R0, R1);
+ auto AddRCLLT_CSE = CSEB.buildInstr(AMDGPU::G_ADD, {{SgprRC, S32}}, {R0, R1});
+ auto AddRBLLT_CSE = CSEB.buildInstr(AMDGPU::G_ADD, {SgprRBS32}, {R0, R1});
+
+ ASSERT_EQ(AddLLT, AddLLT_CSE);
+ ASSERT_EQ(AddRCLLT, AddRCLLT_CSE);
+ ASSERT_EQ(AddRBLLT, AddRBLLT_CSE);
+}
|
arsenm
reviewed
Oct 25, 2024
Comment on lines
136
to
141
| switch (Ty) { | ||
| case DstType::Ty_VRegAttrs: | ||
| return Attrs; | ||
| default: | ||
| llvm_unreachable("Not a VRegAttrs Operand"); | ||
| } |
There was a problem hiding this comment.
assert + return? This will only ever handle one case
| const GISelInstProfileBuilder &GISelInstProfileBuilder::addNodeIDRegType( | ||
| MachineRegisterInfo::VRegAttrs Attrs) const { | ||
| LLT Ty = Attrs.Ty; | ||
| if (Ty.isValid()) |
There was a problem hiding this comment.
This should always be valid
Comment on lines
369
to
370
| else if (const auto *RC = | ||
| dyn_cast_if_present<const TargetRegisterClass *>(RCOrRB)) |
There was a problem hiding this comment.
else and cast, if you're using this version it should be set
| @@ -0,0 +1,75 @@ | |||
| //===- llvm/unittests/Target/AMDGPU/ExecMayBeModifiedBeforeAnyUse.cpp -----===// | |||
| auto AddRCLLT = CSEB.buildInstr(AMDGPU::G_ADD, {SgprRCS32}, {R0, R1}); | ||
| auto AddRBLLT = CSEB.buildInstr(AMDGPU::G_ADD, {{SgprRB, S32}}, {R0, R1}); | ||
|
|
||
| ASSERT_NE(AddLLT, AddRCLLT); |
petar-avramovic
force-pushed
the
Bank+LLT
branch
from
295c13d to
0a7e5fd
Compare
arsenm
reviewed
Oct 29, 2024
| if (const auto *RB = dyn_cast_if_present<const RegisterBank *>(RCOrRB)) | ||
| addNodeIDRegType(RB); | ||
| else | ||
| addNodeIDRegType(dyn_cast<const TargetRegisterClass *>(RCOrRB)); |
There was a problem hiding this comment.
Suggested change
| addNodeIDRegType(dyn_cast<const TargetRegisterClass *>(RCOrRB)); | |
| addNodeIDRegType(cast<const TargetRegisterClass *>(RCOrRB)); |
Comment on lines
59
to
61
| auto AddLLT = CSEB.buildAdd(S32, R0, R1); | ||
| auto AddRCLLT = CSEB.buildInstr(AMDGPU::G_ADD, {SgprRCS32}, {R0, R1}); | ||
| auto AddRBLLT = CSEB.buildInstr(AMDGPU::G_ADD, {{SgprRB, S32}}, {R0, R1}); |
There was a problem hiding this comment.
These aren't LLTs? Remove from the name?
There was a problem hiding this comment.
Name meant what is being looked up by CSE builder, but since they all have LLT, it can be removed from name
Add %2:_(s32) = G_ADD %0:_, %1:_
AddRC %3:sreg_32(s32) = G_ADD %0:_, %1:_
AddRB %4:sgpr(s32) = G_ADD %0:_, %1:_
Allow construction of DstOp with VRegAttrs. Also allow construction with register class or bank and LLT. Intended to be used in lowering code for reg-bank-select where new registers need to have both register bank and LLT. Add support for new type of DstOp in CSEMIRBuilder.
petar-avramovic
force-pushed
the
Bank+LLT
branch
from
0a7e5fd to
7fa22cd
Compare
arsenm
approved these changes
Oct 29, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/1242 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/17/builds/3602 Here is the relevant piece of the build log for the reference |
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow construction of DstOp with VRegAttrs.
Also allow construction with register class or bank and LLT.
Intended to be used in lowering code for reg-bank-select where
new registers need to have both register bank and LLT.
Add support for new type of DstOp in CSEMIRBuilder.