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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

See inline comments.  Otherwise this looks good, but I'd rather commit this as two separate patches (the memory-immediate changes in one, and the MS(G)RKC changes in the other).



================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:3138
   let M4 = 0;
-  let OpKey = mnemonic#cls1;
+  // Change MSRKC and MSGRKC names during tablegen mapping to work with "rk".
+  let OpKey = !subst("msgrkc", "msgcrk", !subst("msrkc", "mscrk", mnemonic))#cls1;
----------------
Ugh, that's annoying.   Can you at least move the mnemonic twiddling to the other side, i.e. to MemFoldPseudo?   We already have similar twiddling for LOC vs SEL there.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1133
 
+  if (Opcode == SystemZ::LEFR) {
+    Register DstReg = MI.getOperand(0).getReg();
----------------
jonpa wrote:
> This handles just some 30-40 cases so I am not sure how useful this is, but I suppose it can be.
> 
> There are a few more (50?) unhandled cases of LEFR/LFER that appear to require things like new opcodes with special handlings or similar ('%gr64bit = LFER %vr32bit' for instance seems awkward to handle ...)
It seems a bit odd to just handle LEFR/LFER (which are simply special forms of VLVGF/VLGVF) while not handling any of the regular VLVG/VLGV forms ...

It's probably not worth having just the special case; maybe we should have logic to handle the generic case.  But in any case this should also be a separate patch.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1228
+  if (SystemZ::getTwoOperandOpcode(Opcode) != -1 ||
+      Opcode == SystemZ::MSRKC || Opcode == SystemZ::MSGRKC) {
     if (VRM == nullptr)
----------------
I'm wondering if the should just have getTwoOperandOpcode return something for these instead.

But it's probably not worth the complication to special-case the CC handling in ShortenInst ...


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

https://reviews.llvm.org/D76055