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

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin marked an inline comment as done.
vpykhtin added inline comments.


================
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.
----------------
arsenm wrote:
> s/to the right/to the left/
I'm not sure why left? We use lower bits of a superreg so this is like shifting from high to low bits and in my opinion this is called right shift.


================
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)
----------------
arsenm wrote:
> 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?
Yes, I was thinking about situation when debug info actually refer to the whole superreg instead of subregs - in this case we can try to make expression that refers to the shifted content. I'm not sure however,  would be nice to talk to debug info experts.


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:91
+  /// Map OldSubReg -> { RC, NewSubReg }. Used as in/out container.
+  typedef SmallDenseMap<unsigned, SubRegInfo> SubRegMap;
+
----------------
arsenm wrote:
> 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 
Ok, but I'm not sure about its size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139732