[AMDGPU] Serialize WWM_REG vreg flag #110229

optimisan · 2024-09-27T09:45:34Z

No description provided.

optimisan · 2024-09-27T09:45:49Z

[AMDGPU] Serialize WWM_REG vreg flag #110229 👈
[MIR] Add missing noteNewVirtualRegister callbacks #111634
[MIR] Serialize virtual register flags #110228
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @optimisan and the rest of your teammates on Graphite

llvmbot · 2024-09-27T11:13:15Z

@llvm/pr-subscribers-backend-amdgpu

Author: Akshat Oke (Akshat-Oke)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/110229.diff

5 Files Affected:

(modified) llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp (+15)
(modified) llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h (+2-2)
(modified) llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp (+11)
(modified) llvm/lib/Target/AMDGPU/SIRegisterInfo.h (+10)
(added) llvm/test/CodeGen/AMDGPU/virtual-registers.mir (+16)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
index abd50748f2cc05..c42b443230acc9 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
@@ -1628,6 +1628,21 @@ bool GCNTargetMachine::parseMachineFunctionInfo(
     MFI->reserveWWMRegister(ParsedReg);
   }
 
+  auto setRegisterFlags = [&](const VRegInfo &Info) {
+    for (const auto &Flag : Info.Flags) {
+      MFI->setFlag(Info.VReg, Flag);
+    }
+  };
+
+  for (const auto &P : PFS.VRegInfosNamed) {
+    const VRegInfo &Info = *P.second;
+    setRegisterFlags(Info);
+  }
+  for (const auto &P : PFS.VRegInfos) {
+    const VRegInfo &Info = *P.second;
+    setRegisterFlags(Info);
+  }
+
   auto parseAndCheckArgument = [&](const std::optional<yaml::SIArgument> &A,
                                    const TargetRegisterClass &RC,
                                    ArgDescriptor &Arg, unsigned UserSGPRs,
diff --git a/llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h b/llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h
index aff0b34947d688..7fffc706c4b027 100644
--- a/llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h
+++ b/llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h
@@ -684,8 +684,8 @@ class SIMachineFunctionInfo final : public AMDGPUMachineFunction,
 
   void setFlag(Register Reg, uint8_t Flag) {
     assert(Reg.isVirtual());
-    if (VRegFlags.inBounds(Reg))
-      VRegFlags[Reg] |= Flag;
+    VRegFlags.grow(Reg);
+    VRegFlags[Reg] |= Flag;
   }
 
   bool checkFlag(Register Reg, uint8_t Flag) const {
diff --git a/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp b/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
index 2d1cd1bda3afe1..ad3e592127de80 100644
--- a/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
@@ -3614,3 +3614,14 @@ SIRegisterInfo::getSubRegAlignmentNumBits(const TargetRegisterClass *RC,
   }
   return 0;
 }
+
+SmallVector<std::string>
+SIRegisterInfo::getVRegFlagsOfReg(Register Reg,
+                                  const MachineFunction &MF) const {
+  SmallVector<std::string> RegFlags;
+  const SIMachineFunctionInfo *FuncInfo = MF.getInfo<SIMachineFunctionInfo>();
+  if (FuncInfo->checkFlag(Reg, AMDGPU::VirtRegFlag::WWM_REG)) {
+    RegFlags.push_back("WWM_REG");
+  }
+  return RegFlags;
+}
diff --git a/llvm/lib/Target/AMDGPU/SIRegisterInfo.h b/llvm/lib/Target/AMDGPU/SIRegisterInfo.h
index 88d5686720985e..bce5a2aa792bd3 100644
--- a/llvm/lib/Target/AMDGPU/SIRegisterInfo.h
+++ b/llvm/lib/Target/AMDGPU/SIRegisterInfo.h
@@ -449,6 +449,16 @@ class SIRegisterInfo final : public AMDGPUGenRegisterInfo {
   // No check if the subreg is supported by the current RC is made.
   unsigned getSubRegAlignmentNumBits(const TargetRegisterClass *RC,
                                      unsigned SubReg) const;
+
+  std::pair<bool, uint8_t> getVRegFlagValue(StringRef Name) const override {
+    if (Name == "WWM_REG") {
+      return {true, AMDGPU::VirtRegFlag::WWM_REG};
+    }
+    return {false, 0};
+  }
+
+  SmallVector<std::string>
+  getVRegFlagsOfReg(Register Reg, const MachineFunction &MF) const override;
 };
 
 namespace AMDGPU {
diff --git a/llvm/test/CodeGen/AMDGPU/virtual-registers.mir b/llvm/test/CodeGen/AMDGPU/virtual-registers.mir
new file mode 100644
index 00000000000000..3ea8f6eafcf10c
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/virtual-registers.mir
@@ -0,0 +1,16 @@
+# RUN: llc -mtriple=amdgcn -run-pass=none -o - %s | FileCheck %s
+# This test ensures that the MIR parser parses virtual register flags correctly
+
+---
+name: vregs
+# CHECK: registers:
+# CHECK-NEXT:   - { id: 0, class: vgpr_32, preferred-register: '$vgpr1', flags: [ WWM_REG ] }
+# CHECK-NEXT:   - { id: 1, class: sgpr_64, preferred-register: '$sgpr0_sgpr1', flags: [  ] }
+# CHECK-NEXT:   - { id: 2, class: sgpr_64, preferred-register: '', flags: [  ] }
+registers:
+  - { id: 0, class: vgpr_32, preferred-register: $vgpr1, flags: [ WWM_REG ]}
+  - { id: 1, class: sgpr_64, preferred-register: $sgpr0_sgpr1 }
+body: |
+  bb.0:
+    %2:sgpr_64 = COPY %1
+    %1:sgpr_64 = COPY %0

cdevadas · 2024-09-28T05:10:38Z

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h

@@ -684,8 +684,8 @@ class SIMachineFunctionInfo final : public AMDGPUMachineFunction,

  void setFlag(Register Reg, uint8_t Flag) {
    assert(Reg.isVirtual());
-    if (VRegFlags.inBounds(Reg))
-      VRegFlags[Reg] |= Flag;
+    VRegFlags.grow(Reg);


I guess, this change is unnecessary. Keep the inbounds check back.
The following change in the other patch would make sure to grow VRegFlags for each virtual register it encountered.
https://github.com/llvm/llvm-project/pull/110228/files#diff-c72079b2a595aca3300d5e3c15d227f81937f2745f7c5494fcf1fe9ba37d8828R1789

The MIR function is parsed after parsing the options, so the noteNewVirtualRegister callback doesn't take effect.

You changed this though?

It doesn't take effect here.
Might be better to move it to a separate commit.

This change doesn't make sense to me. What will happen to the regular flow when it reaches from MRI createVirtualRegister? Isn't duplicating the size?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/MachineRegisterInfo.cpp#L166

I think Register info is an avenue for noting new virtual registers so I've put it here #111634

cdevadas · 2024-09-28T05:15:45Z

llvm/test/CodeGen/AMDGPU/virtual-registers.mir

@@ -0,0 +1,16 @@
+# RUN: llc -mtriple=amdgcn -run-pass=none -o - %s | FileCheck %s


Mostly, the tests related to serialize options go in llvm/test/CodeGen/MIR/AMDGPU/machine-function-info-no-ir.mir.
Additionally, a negative test is required for this serialized option and that should be a separate test like llvm/test/CodeGen/MIR/AMDGPU/sgpr-for-exec-copy-invalid-reg.mir

Negative test is now in MIR/Generic.

arsenm · 2024-09-30T16:10:16Z

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

@@ -1628,6 +1628,21 @@ bool GCNTargetMachine::parseMachineFunctionInfo(
    MFI->reserveWWMRegister(ParsedReg);
  }

+  auto setRegisterFlags = [&](const VRegInfo &Info) {
+    for (const auto &Flag : Info.Flags) {


No auto, no reference. This is just uint8_t

arsenm · 2024-09-30T16:10:30Z

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

+  for (const auto &P : PFS.VRegInfosNamed) {
+    const VRegInfo &Info = *P.second;


c++17 destructuring

arsenm · 2024-09-30T16:10:38Z

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

+  for (const auto &P : PFS.VRegInfos) {
+    const VRegInfo &Info = *P.second;


c++17 destructuring

arsenm · 2024-09-30T16:11:20Z

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

@@ -3614,3 +3614,14 @@ SIRegisterInfo::getSubRegAlignmentNumBits(const TargetRegisterClass *RC,
  }
  return 0;
 }
+
+SmallVector<std::string>


probably should just be const char*, this will probably only ever be used with literals

llvm/test/CodeGen/AMDGPU/virtual-registers.mir

llvm/test/CodeGen/MIR/AMDGPU/machine-function-info-no-ir.mir

arsenm · 2024-10-08T07:01:42Z

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

+  if (FuncInfo->checkFlag(Reg, AMDGPU::VirtRegFlag::WWM_REG)) {
+    RegFlags.push_back(SmallString<8>("WWM_REG"));
+  }


Suggested change

if (FuncInfo->checkFlag(Reg, AMDGPU::VirtRegFlag::WWM_REG)) {

RegFlags.push_back(SmallString<8>("WWM_REG"));

}

if (FuncInfo->checkFlag(Reg, AMDGPU::VirtRegFlag::WWM_REG))

RegFlags.push_back("WWM_REG");

I would hope construct from literal works fine

Should I change this to a const char* instead?

I expect only literals, so could use StringRef or StringLiteral

Alright, will keep this.
I have to keep the explicit construct since "WWM_REG" is being refused to be casted to a SmallString (without creating a StringRef explicitly)

I mean the interface could change from SmallString to StringLiteral

arsenm · 2024-10-08T07:02:04Z

llvm/lib/Target/AMDGPU/SIRegisterInfo.h

+    if (Name == "WWM_REG") {
+      return AMDGPU::VirtRegFlag::WWM_REG;
+    }
+    return {};


Suggested change

if (Name == "WWM_REG") {

return AMDGPU::VirtRegFlag::WWM_REG;

}

return {};

return Name == "WWM_REG" ? AMDGPU::VirtRegFlag::WWM_REG : {};

arsenm · 2024-10-08T07:02:24Z

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h

@@ -684,8 +684,8 @@ class SIMachineFunctionInfo final : public AMDGPUMachineFunction,

  void setFlag(Register Reg, uint8_t Flag) {
    assert(Reg.isVirtual());
-    if (VRegFlags.inBounds(Reg))
-      VRegFlags[Reg] |= Flag;
+    VRegFlags.grow(Reg);


You changed this though?

arsenm · 2024-10-08T19:10:43Z

llvm/lib/Target/AMDGPU/SIRegisterInfo.h

+    return (Name == "WWM_REG") ? AMDGPU::VirtRegFlag::WWM_REG
+                               : std::optional<uint8_t>{};


Suggested change

return (Name == "WWM_REG") ? AMDGPU::VirtRegFlag::WWM_REG

: std::optional<uint8_t>{};

return Name == "WWM_REG" ? AMDGPU::VirtRegFlag::WWM_REG

: std::optional<uint8_t>{};

arsenm · 2024-10-09T06:47:49Z

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

+    for (uint8_t Flag : Info.Flags) {
+      MFI->setFlag(Info.VReg, Flag);
+    }


Might as well just inline this and duplicate the code, the loop is trivial enough

arsenm · 2024-10-09T06:48:11Z

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp

+  if (FuncInfo->checkFlag(Reg, AMDGPU::VirtRegFlag::WWM_REG)) {
+    RegFlags.push_back("WWM_REG");
+  }


Suggested change

if (FuncInfo->checkFlag(Reg, AMDGPU::VirtRegFlag::WWM_REG)) {

RegFlags.push_back("WWM_REG");

}

if (FuncInfo->checkFlag(Reg, AMDGPU::VirtRegFlag::WWM_REG))

RegFlags.push_back("WWM_REG");

pcc · 2024-10-14T23:04:32Z

It looks like this change caused LSan reports. See e.g. https://lab.llvm.org/buildbot/#/builders/52/builds/2928

Can you please take a look?

optimisan · 2024-10-15T02:55:03Z

Taking a look

This reverts commit bec839d. Caused buildbot failures, e.g. https://lab.llvm.org/buildbot/#/builders/52/builds/2928

pcc · 2024-10-15T20:20:32Z

I reverted the change for now, please reland when you have a fix.

optimisan · 2024-10-16T05:15:03Z

Yep
Have a fix here #112479

This reverts commit bec839d. Caused buildbot failures, e.g. https://lab.llvm.org/buildbot/#/builders/52/builds/2928

A reland but not an exact copy as `VRegInfo.Flags` from the parser is now an int8 instead of a vector; so only need to copy over the value.

optimisan mentioned this pull request Sep 27, 2024

[MIR] Serialize virtual register flags #110228

Merged

optimisan marked this pull request as ready for review September 27, 2024 11:12

llvmbot added the backend:AMDGPU label Sep 27, 2024

cdevadas reviewed Sep 28, 2024

View reviewed changes

cdevadas requested review from arsenm and pravinjagtap September 28, 2024 05:26

arsenm mentioned this pull request Sep 30, 2024

[AMDGPU] When allocating VGPRs, VGPR spills are not part of the prologue #109439

Merged

arsenm reviewed Sep 30, 2024

View reviewed changes

optimisan force-pushed the users/Akshat-Oke/09-27-_mir_serialize_virtual_register_flags branch from f494d56 to 336b9bc Compare October 4, 2024 06:40

optimisan force-pushed the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch from 35536a8 to c428da0 Compare October 4, 2024 06:40

arsenm reviewed Oct 4, 2024

View reviewed changes

llvm/test/CodeGen/MIR/AMDGPU/machine-function-info-no-ir.mir Show resolved Hide resolved

optimisan force-pushed the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch from c428da0 to 6651bfa Compare October 8, 2024 06:31

arsenm reviewed Oct 8, 2024

View reviewed changes

optimisan force-pushed the users/Akshat-Oke/09-27-_mir_serialize_virtual_register_flags branch from 9f8b6eb to 7af9e5e Compare October 9, 2024 05:57

optimisan force-pushed the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch from 81baf9e to 79de29b Compare October 9, 2024 05:57

optimisan changed the base branch from users/Akshat-Oke/09-27-_mir_serialize_virtual_register_flags to users/Akshat-Oke/10-09-_mir_add_missing_notenewvirtualregister_callbacks October 9, 2024 05:58

optimisan mentioned this pull request Oct 9, 2024

[MIR] Add missing noteNewVirtualRegister callbacks #111634

Merged

optimisan force-pushed the users/Akshat-Oke/10-09-_mir_add_missing_notenewvirtualregister_callbacks branch from 2b87714 to 8e7f366 Compare October 9, 2024 06:16

optimisan force-pushed the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch from 79de29b to 81059dc Compare October 9, 2024 06:16

arsenm approved these changes Oct 9, 2024

View reviewed changes

cdevadas approved these changes Oct 9, 2024

View reviewed changes

optimisan force-pushed the users/Akshat-Oke/10-09-_mir_add_missing_notenewvirtualregister_callbacks branch from bf5cc39 to 9dd1158 Compare October 14, 2024 08:21

optimisan force-pushed the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch from 81059dc to db80f0b Compare October 14, 2024 08:21

optimisan force-pushed the users/Akshat-Oke/10-09-_mir_add_missing_notenewvirtualregister_callbacks branch from 9dd1158 to 9390926 Compare October 14, 2024 08:31

optimisan force-pushed the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch from db80f0b to 3240cf9 Compare October 14, 2024 08:31

optimisan force-pushed the users/Akshat-Oke/10-09-_mir_add_missing_notenewvirtualregister_callbacks branch from 9390926 to 38a9c7b Compare October 14, 2024 08:51

optimisan force-pushed the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch from 3240cf9 to 4870a8a Compare October 14, 2024 08:52

optimisan added 5 commits October 14, 2024 09:03

AS

083a87c

conditional

d24baa9

StringLiteral

7d60273

.grow no longer required

318e2bd

Apply suggestions

0712903

optimisan force-pushed the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch from 32b44f5 to 0712903 Compare October 14, 2024 09:03

optimisan merged commit bec839d into main Oct 14, 2024
5 of 7 checks passed

optimisan deleted the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch October 14, 2024 09:07

pcc added a commit that referenced this pull request Oct 15, 2024

Revert "[AMDGPU] Serialize WWM_REG vreg flag (#110229)"

3cab882

This reverts commit bec839d. Caused buildbot failures, e.g. https://lab.llvm.org/buildbot/#/builders/52/builds/2928

optimisan mentioned this pull request Oct 16, 2024

[MIR] Fix vreg flag vector memory leak #112479

Merged

optimisan restored the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch October 16, 2024 07:22

optimisan deleted the users/Akshat-Oke/09-27-_amdgpu_serialize_wwm_reg_vreg_flag branch October 16, 2024 07:42

optimisan added a commit to optimisan/llvm-project that referenced this pull request Oct 16, 2024

Reland [AMDGPU] Serialize WWM_REG vreg flag (llvm#110229)

cd8df5d

optimisan added a commit to optimisan/llvm-project that referenced this pull request Oct 16, 2024

Reland [AMDGPU] Serialize WWM_REG vreg flag (llvm#110229)

d652118

DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024

[AMDGPU] Serialize WWM_REG vreg flag (llvm#110229)

916f222

DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024

Revert "[AMDGPU] Serialize WWM_REG vreg flag (llvm#110229)"

35dd8b7

This reverts commit bec839d. Caused buildbot failures, e.g. https://lab.llvm.org/buildbot/#/builders/52/builds/2928

optimisan added a commit to optimisan/llvm-project that referenced this pull request Oct 21, 2024

Reland [AMDGPU] Serialize WWM_REG vreg flag (llvm#110229)

1ed2b06

optimisan added a commit that referenced this pull request Oct 21, 2024

Reland [AMDGPU] Serialize WWM_REG vreg flag (#110229) (#112492)

6360652

A reland but not an exact copy as `VRegInfo.Flags` from the parser is now an int8 instead of a vector; so only need to copy over the value.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMDGPU] Serialize WWM_REG vreg flag #110229

[AMDGPU] Serialize WWM_REG vreg flag #110229

optimisan commented Sep 27, 2024

optimisan commented Sep 27, 2024 •

edited

Loading

llvmbot commented Sep 27, 2024

cdevadas Sep 28, 2024

optimisan Sep 30, 2024

arsenm Oct 8, 2024

optimisan Oct 8, 2024

cdevadas Oct 8, 2024

optimisan Oct 9, 2024

cdevadas Sep 28, 2024

optimisan Oct 4, 2024

arsenm Sep 30, 2024

arsenm Sep 30, 2024

arsenm Sep 30, 2024

arsenm Sep 30, 2024

arsenm Oct 8, 2024

optimisan Oct 8, 2024

arsenm Oct 8, 2024

optimisan Oct 8, 2024

arsenm Oct 8, 2024

optimisan Oct 8, 2024

arsenm Oct 8, 2024

arsenm Oct 8, 2024

arsenm Oct 8, 2024

arsenm Oct 9, 2024

arsenm Oct 9, 2024

pcc commented Oct 14, 2024

optimisan commented Oct 15, 2024 •

edited

Loading

pcc commented Oct 15, 2024

optimisan commented Oct 16, 2024 •

edited

Loading

		@@ -0,0 +1,16 @@
		# RUN: llc -mtriple=amdgcn -run-pass=none -o - %s \| FileCheck %s

		for (const auto &P : PFS.VRegInfosNamed) {
		const VRegInfo &Info = *P.second;

		for (const auto &P : PFS.VRegInfos) {
		const VRegInfo &Info = *P.second;

		return (Name == "WWM_REG") ? AMDGPU::VirtRegFlag::WWM_REG
		: std::optional<uint8_t>{};

[AMDGPU] Serialize WWM_REG vreg flag #110229

[AMDGPU] Serialize WWM_REG vreg flag #110229

Conversation

optimisan commented Sep 27, 2024

optimisan commented Sep 27, 2024 • edited Loading

llvmbot commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcc commented Oct 14, 2024

optimisan commented Oct 15, 2024 • edited Loading

pcc commented Oct 15, 2024

optimisan commented Oct 16, 2024 • edited Loading

optimisan commented Sep 27, 2024 •

edited

Loading

optimisan commented Oct 15, 2024 •

edited

Loading

optimisan commented Oct 16, 2024 •

edited

Loading