[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 7 inline comments as done.
vpykhtin added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:81
+    /// Register class required to hold the value stored in the SubReg.
+    const TargetRegisterClass *RC;
+
----------------
arsenm wrote:
> Should use a const reference, this should never need to be null 
RC is gradually refined with getCommonSubClass, please see line 418.


================
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:
> 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.


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:220
+
+    // Instruction operand may not specify required register class (ex. COPY).
+    if (!SubRegRC)
----------------
arsenm wrote:
> We have helpers to query the class from the instruction, this shouldn't be reachable?
Would be appreciated for the function name hint.


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:257
+  // allocatable, aligned, has all shifted subregs and each subreg has required
+  // register class (see SubRegRC above). Now select first (that is largest)
+  // register class with registers of minimal size.
----------------
arsenm wrote:
> I thought the classes were already supposed to be sorted with largest first such that you only need to look at the first set bit?
They're sorted by the number of registers in a class but we need to find the largest class that has registers of minimally suitable size.


================
Comment at: llvm/lib/Target/AMDGPU/GCNRewritePartialRegUses.cpp:391
+    NewLI.assign(OldLI, Allocator);
+  NewLI.verify(MRI);
+  LIS->removeInterval(OldReg);
----------------
arsenm wrote:
> I don't think verify is defined in a release build
It is defined with an empty body.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139732