[PATCH] D139732: [AMDGPU] Add pass to rewrite partially used virtual superregisters after RenameIndependentSubregs pass with registers of minimal size.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added a comment.
This revision is now accepted and ready to land.

LGTM with some nits (plus I would still prefer if we generated static tables for the compacted subreg mappings)



================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:74
+  /// Rewrite partially used register Reg by shifting all its subregisters to
+  /// the right and replacing the original register with a register of minimal
+  /// size. Return true if the change has been made.
----------------
s/to the right/to the left/


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:83
+
+    /// Index for the right-shifted subregister. If 0 this is the "covering"
+    /// subreg i.e. subreg that covers all others. Covering subreg becomes the
----------------
Left


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:415
+    auto *OpDescRC = getOperandRegClass(MO);
+    const auto &[I, Inserted] = SubRegs.try_emplace(MO.getSubReg(), OpDescRC);
+    if (!Inserted) {
----------------
No &


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:439
+    MO.setReg(NewReg);
+    // Debug info can refer to the whole reg, just left it as it is for now.
+    // TODO: create some DI shift expression?
----------------
s/left/leave/


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:440
+    // Debug info can refer to the whole reg, just left it as it is for now.
+    // TODO: create some DI shift expression?
+    if (MO.isDebug() && MO.getSubReg() == 0)
----------------
I don't understand why debug info needs special consideration here. If it was referring to a virtual register before, it can refer to the adjusted virtual register?


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:91
+  /// Map OldSubReg -> { RC, NewSubReg }. Used as in/out container.
+  typedef SmallDenseMap<unsigned, SubRegInfo> SubRegMap;
+
----------------
vpykhtin wrote:
> arsenm wrote:
> > vpykhtin wrote:
> > > arsenm wrote:
> > > > Shouldn't this be a static table?
> > > I agree, but sometime ago I was warned about avoiding static data-members to reduce library memory footprint.
> > This wouldn't even be that big of a table. Tablegen could emit it. The concern would be static constructors, a simple static constexpr table wouldn't have that issue.
> Sorry this isn't constant map, I was thinking about cashed tables. This map is only valid for a specific combination of subregs used: for example, if we use only sub3, we can shift it to sub0, but if we use sub2_sub3, sub2 and sub3 - sub3 can only be sub1.
But this set of constraints could be represented in a static table 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139732/new/

https://reviews.llvm.org/D139732