[PATCH] D67437: [SystemZ] Improve foldMemoryOperandImpl().

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

In D67437#1812901 <https://reviews.llvm.org/D67437#1812901>, @jonpa wrote:

> Patch extended to also fold reloads into conditional load operands (both LOCR and SELR).


OK, makes sense.

> There are two new MemFoldPseudo opcodes that are not used since we always use LOCRMux : LOCFH_MemFoldPseudo and LOC_MemFoldPseudo.

Then we really shouldn't generate them --- see inline comments.

> Perhaps just reuse reverseCCMask() in SystemZISelLowering.cpp instead of the new SystemZ::getSwappedCCMask() ..?

Yes, agreed.  See also inline comments.



================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:2846
-  def Asm : AsmCondUnaryRSY<mnemonic, opcode, cls, bytes, mode>;
-}
-
----------------
I think it would be better to leave this here (to allow for selectively creating _MemFoldPseudo only when we need it).  Also, for consistency, we should then set OpKey/OpType in CondUnaryRSY.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:3231
                              RegisterOperand cls1, RegisterOperand cls2> {
-  let isCodeGenOnly = 1 in
+  let isCodeGenOnly = 1, OpKey = mnemonic#cls1, OpType = "reg" in
     def "" : CondBinaryRRF<mnemonic, opcode, cls1, cls2>;
----------------
Move OpKey and OpType into CondBinaryRRF.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:3246
   let NumOpsValue = "3";
+  let OpKey = mnemonic;
+  let OpType = "reg";
----------------
For consistency, also append cls1 to mnemonic here.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:4776
+           (ins cls:$R2, mode:$XBD2, cond4:$valid, cond4:$M3), []> {
+    let OpKey = !subst("loc", "selr", !subst("locg", "selgr", mnemonic));
+    let OpType = "mem";
----------------
Append cls.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:4778
+    let OpType = "mem";
+    let MemKey = mnemonic;
+    let MemType = "pseudo";
----------------
Append cls.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:4823
   let NumOpsValue = "2";
+  let OpKey = mnemonic;
+  let OpType = "reg";
----------------
Append cls1.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:4838
   let NumOpsValue = "3";
+  let OpKey = mnemonic;
+  let OpType = "reg";
----------------
Append cls1.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:4876
+  def _MemFoldPseudo : MemFoldPseudo_CondMove<mnemonic, cls, bytes, mode>;
 }
 
----------------
Again, I think it would be better to leave this as-is, except for adding OpKey/OpType, and then adding another multiclass below that creates the MemFoldPseudo.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:5097
+  def _MemFoldPseudo : MemFoldPseudo_CondMove<mnemonic, cls, bytes, mode>;
+}
+
----------------
This should have a different name that makes it clear that it creates a MemFold pseudo.  Maybe "CondUnaryRSYPairAndMemFold"   (ideally, we should rename the other MemFold pseudo multiclasses to match).

We shouldn't set OpKey/OpType here, but in CondUnaryRSY.

In fact, we may want to also move setting of MemKey/MemType (for MemType "target") into the original classes as well, then we don't need to duplicate stuff here.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:5098
+}
+
 // Define an instruction that operates on two fixed-length blocks of memory,
----------------
Also, we need a CondUnaryRSYPseudoAndMemFold analogously here.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1793
+int32_t SystemZ::getSwappedCCMask(int32_t InCCMask) {
+  switch(InCCMask) {
+  case SystemZ::CCMASK_CMP_LT:    return SystemZ::CCMASK_CMP_GT;
----------------
You're right, this really should use the code in reverseCCMask -- then we also wouldn't have to deal with any failure cases.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.td:534
   // Expands to LOC or LOCFH, depending on the choice of register.
-  def LOCMux : CondUnaryRSYPseudo<simple_load, GRX32, 4>;
+  defm LOCMux : CondUnaryRSYPseudo<"locmux", simple_load, GRX32, 4>;
   defm LOCFH : CondUnaryRSYPair<"locfh", 0xEBE0, simple_load, GRH32, 4>;
----------------
Should use ...AndMemFold exactly there where we need them.

Also, the "mnemonic" for mux pseudos should probably be something like "MUXloc", then can keep simply appending "r" without extra hassle.


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

https://reviews.llvm.org/D67437