[PATCH] D67437: [SystemZ] Swap compare operands in foldMemoryOperandImpl() if possible.

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

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

> Patch rebased and using LivePhysRegs. New tests added.
>
> I thought about using a CutOff counter in the search for CC Users (a value of 50 is NFC on SPEC 2006). But then I thought that maybe this isn't really needed since many instructions define CC at which point the search stops...


Agreed.

> I also realized that we could do WFCDB -> CDBR -> CDB, WFCSB -> CEBR -> CEB if the non-spilled reg is allocated to FP bank (and possibly also constrain a non-allocated reg to FP reg). This compare could also be swapped since the CC=3 for FP compares would not have to be handled, right? I am thinking this could wait until this patch has been committed, but not sure if it would be better to do both at the same time and then run benchmarks just one time...

We definitely can swap FP compares.  Converting WFCDB to FP is a bit of a trade-off since you need to restrict the register bank ... that probably needs more performance verification.  I agree extending this to FP can be done in a separate patch.

Re-added some inline comments that seem to have been lost ...



================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1730
+  if (LiveRegs.contains(SystemZ::CC))
+    return false;
+
----------------
I believe we do **not** need this check if the loop below finds another CC def after MI.  We only need to check live-outs if this MI is the last CC def in the basic block.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1745
+  }
+  assert(CCUsers.size() && "No CC users found?");
+
----------------
I'm not sure it is safe to assert here. If there's no CC users of a compare, then the compare is dead and will usually have been optimized away. But I'm not sure we can fully rely on that to have happened in all cases (e.g. what about -O0?).


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1753
+    MachineOperand &CCMaskMO = CCUsers[Idx]->getOperand(FirstOpNum + 1);
+    switch(CCMaskMO.getImm()) {
+    case SystemZ::CCMASK_CMP_LT:
----------------
This logic duplicates the CC commutation operation done in SystemZISelLowering::combineCCMask, so it would be preferable to have a helper routine somewhere.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1776
+  // Swap the registers and flags of the compare operands. MBBI is expected
+  // to remain without constructing a new one.
+  MachineOperand &LHS = MBBI->getOperand(0);
----------------
Why do you need to modify the instruction here in the first place? The caller throws it away anyway and creates the memory variant. Should this just do the commutation in place there?


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

https://reviews.llvm.org/D67437