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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

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

> Patch updated per review (NFC). The new unused opcodes are gone, and the only superfluous mappings now added are in getMemOpcode():
>
>   LOCFHR -> LOCFH
>   LOCR   -> LOC
>  
>
>
> (which are not really "wrong")


Yes, I think this is fine.

> Using reverseCCMask() includes the overflow bit being handled as well (and not checked for), but I suppose this must be equivalent since the compares never set the OF bit, right? (in combineCCMask and prepareCompareSwapOperands). It seems that we are with this patch using reverseCCMask() only for CCUsers in the presence of a compare. So to me it would make more sense to have an assert in reverseCCMask against the OF bit set, rather than handling it. This would be more overall readable given that we do introduce the use of the OF bit in SystemZElimCompare later on (or maybe I am missing something?). In other words, make it explit that the CCMask is produced by a compare and nothing else, like getSwappedCCMask() did. Would we want to use the OF (unordered bit) with FP compares?

Yes, we need the unordered bit handling for FP compares.  For integer compares the bit is never set, so I think this is still fine ...

> Changed mnemonics per suggestion to MUX... This simplifies one line where just an "r" can be added, but the nested subst() in MemFoldPseudo_CondMove is not remedied. Can we still call this string 'mnemonic', or should it 'mnem' or something to show that it is not the same as the name of the instruction (on the other hand, it is already a pseudo opcode...)?

See inline comment, I think this **can** now be simplified.

Except for the minor tweaks pointed on inline, this now LGTM.  Thanks!



================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:4791
+           (ins cls:$R2, mode:$XBD2, cond4:$valid, cond4:$M3), []> {
+    let OpKey = !subst("loc", "selr", !subst("locg", "selgr", mnemonic))#cls;
+    let OpType = "mem";
----------------
I believe this can now simply be
  let OpKey = !subst("loc", "sel", mnemonic)#"r"#cls;


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:5106
+    def "" : CondUnaryRSY<mnemonic, opcode, operator, cls, bytes, mode>;
+  def Asm : AsmCondUnaryRSY<mnemonic, opcode, cls, bytes, mode>;
+  def _MemFoldPseudo : MemFoldPseudo_CondMove<mnemonic, cls, bytes, mode>;
----------------
You should now be able to simply use
  def "" : CondUnaryRSYPair<...>
right?



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

https://reviews.llvm.org/D67437