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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1730
+  if (LiveRegs.contains(SystemZ::CC))
+    return false;
+
----------------
uweigand wrote:
> 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.
done


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:1745
+  }
+  assert(CCUsers.size() && "No CC users found?");
+
----------------
uweigand wrote:
> 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?).
OK


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


================
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);
----------------
uweigand wrote:
> 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?
The high-muxes patch does the exact same thing in PostRewrite in expandCmpMux() (to handle the "low-high" case), so I thought it might be a nice function to have. But I can see that it makes things easier here to instead use NeedsCommute as then we don't need to swap the operands anymore like you suggest.



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

https://reviews.llvm.org/D67437