[PATCH] D133723: [AMDGPU][GFX11] Use VGPR_32_F128 for VOP1,2,C

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

In D133723#3791006 <https://reviews.llvm.org/D133723#3791006>, @Joe_Nash wrote:

> Update commit message for renaming and the separate patch disabling pre-RA shrinking.

Can you copy the new commit message into the summary of this patch? You have to do that manually.



================
Comment at: llvm/lib/Target/AMDGPU/SIInstructions.td:987
+let SubtargetPredicate = HasTrue16BitInsts in
+defm : f16_fp_Pats<V_CVT_F16_F32_T16_e64, V_CVT_F32_F16_T16_e64>;
 
----------------
Joe_Nash wrote:
> dp wrote:
> > Why are these patterns special? `_E64` variants have no VGPR limitations. Is this required for future changes?
> This is simply to account for the fact that we have new pseudo instructions for each instruction with 16 bit operands on GFX11. Therefore if a pattern directly directly refers to an instruction we need to duplicate it, one for each pseudo. 
> Is this required for future changes?

Yes. In future the _t16_e64 version will use 16-bit register classes for 16-bit operands but the regular _e64 version will continue to use 32-bit classes for them.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.td:557
+
+def VGPR_32_F128 : SIRegisterClass<"AMDGPU", !listconcat(Reg32Types.types, Reg16Types.types), 32,
+                            (add (sequence "VGPR%u", 0, 127))> {
----------------
Joe_Nash wrote:
> rampitec wrote:
> > arsenm wrote:
> > > Joe_Nash wrote:
> > > > arsenm wrote:
> > > > > I don't know what "_F128" is supposed to mean. I read this as a class for long double
> > > > It is short for First 128. Is a set with only the first 128 VGPRs. I will add a comment noting this. 
> > > Lo128 would probably be more consistent terminology over "first"
> > I probably agree to that. F128 also hints a long double to me.
> > With that I still hope this class is transitional and will be replaced by a real 16 bit RC.
> Ok, replaced _F128 with _Lo128. 
> Yes, this is a transitional class.
Just saying: Lo128 is not ideal either because it will be confused with the _LO16/HI16 classes which mean something completely different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133723