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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1133
 
+  if (Opcode == SystemZ::LEFR) {
+    Register DstReg = MI.getOperand(0).getReg();
----------------
uweigand wrote:
> 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.
OK, removing that for now since that's relatively rare.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1228
+  if (SystemZ::getTwoOperandOpcode(Opcode) != -1 ||
+      Opcode == SystemZ::MSRKC || Opcode == SystemZ::MSGRKC) {
     if (VRM == nullptr)
----------------
uweigand wrote:
> uweigand wrote:
> > 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 ...
> Hmm.  Thinking about this some more, I think what we're really testing for here is simple: does the MemOpcode need its output and first input operand to be tied or not.  If yes, we need to verify that they can be tied (potentially via commutation).
> 
> I believe it should be possibly to simply check for that property by looking at the MCInstrDest for MemOpcode.
That seems to work but then also the MemFoldPseudo needs to be looked-through.


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

https://reviews.llvm.org/D76055