[PATCH] D144925: [RISCV][NFC] Replace the pseudos for instructions that depend on lmul with variants that encode the SEW into the name

[Nitin John Raj via Phabricator via llvm-commits]

nitinjohnraj added a comment.

In D144925#4157772 <https://reviews.llvm.org/D144925#4157772>, @frasercrmck wrote:

> Just a glancing review but, to me, "expand pseudos" has a more immediate connotation of the many pseudo expansion passes (e.g., `RISCVExpandPseudoInsts`) than "expanding the set of pseudos" as you seem to intend it.

Thanks for the comment, that makes sense. :)



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:1919
+
+        defvar suffix = "_" # m.MX # "_E" # e;
+        def _VM # suffix : VPseudoUnaryAnyMask<m.vrclass, m.vrclass>,
----------------
michaelmaitland wrote:
> If we're going to use suffix, should we use it in `WriteVCompressV_MX_E` and `ReadVCompressV_MX_E` too?
Good point, yeah.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:2041-2061
   foreach m = MxList in {
     defvar mx = m.MX;
-    defvar WriteVGatherV_MX = !cast<SchedWrite>("WriteVGatherV_" # mx);
-    defvar ReadVGatherV_MX = !cast<SchedRead>("ReadVGatherV_" # mx);
 
     foreach sew = EEWList in {
       defvar octuple_lmul = m.octuple;
       // emul = lmul * eew / sew
       defvar octuple_emul = !srl(!mul(octuple_lmul, eew), log2<sew>.val);
----------------
michaelmaitland wrote:
> `WriteVGatherV_MX_E` and `ReadVGatherV_MX_E` dont depend on `sew` from `EEWList`. Can we pull out the loop over `sews` from `SchedSEWSet<mx>.val` to simplify?
Thanks for the review! We can certainly do that, but does it really simplify the code? It's a nested loop in either case, so I'm not clear which version performs better/looks simpler.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:2337
+        defvar ReadVFSqrtV_MX = !cast<SchedRead>("ReadVFSqrtV_" # mx # "_E" # e);
+        defvar suffix = mx # "_E" # e;
+
----------------
michaelmaitland wrote:
> Use `suffix` in `WriteVFSqrtV_MX` and `ReadVFSqrtV_MX`. Also need to add `_E` to  `WriteVFSqrtV_MX` and `ReadVFSqrtV_MX`
Thanks, will do!


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:2468
     defvar mx = m.MX;
-    defvar WriteVGatherV_MX = !cast<SchedWrite>("WriteVGatherV_" # mx);
+    defvar sews = SchedSEWSet<mx>.val;
     defvar WriteVGatherX_MX = !cast<SchedWrite>("WriteVGatherX_" # mx);
----------------
michaelmaitland wrote:
> nit: define `sews` closer to where you use it?
Appreciated, I agree with that nit, seems better for clarity.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:2472
     defvar ReadVGatherX_MX = !cast<SchedRead>("ReadVGatherX_" # mx);
+    defvar ReadVGatherI_MX = !cast<SchedRead>("ReadVGatherI_" # mx);
 
----------------
michaelmaitland wrote:
> Why did we change `ReadVGatherV` to `ReadVGatherI`?
After discussion with @craig.topper, we did this because otherwise we'd need to maintain both SEW-aware and SEW-unaware versions of `ReadVGatherV`:
1. The SEW-aware version on line 2481 for the `_VV` binary, which is SEW-aware itself
2. The SEW-unaware version for the `_VI` binary, which is not SEW aware

This change helps us generate only the SEW-aware `ReadVGatherV` SchedReads. I'd be glad to talk about this more if you think we should approach this differently though.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:2476
     defm "" : VPseudoBinaryV_VX<m, Constraint>,
-              Sched<[WriteVGatherX_MX, ReadVGatherV_MX, ReadVGatherX_MX, ReadVMask]>;
+              Sched<[WriteVGatherX_MX, ReadVGatherX_MX, ReadVGatherX_MX, ReadVMask]>;
     defm "" : VPseudoBinaryV_VI<ImmType, m, Constraint>,
----------------
I feel particularly uneasy about this change. I could leave it as is, or I could create ReadVGatherX_MX with no SEW in RISCVScheduleV.td. 


================
Comment at: llvm/lib/Target/RISCV/RISCVScheduleV.td:41
+    if !eq(mx, "UpperBound") then
+      def name # "_" # mx : SchedWrite;
+    else
----------------
michaelmaitland wrote:
> Why don't we make this case SEW aware?
If I remember correctly, that was causing some other files to complain and I put this in as 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144925