[PATCH] D76705: [SystemZ] Improve foldMemoryOperandImpl: vec->FP conversions

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

In D76705#2029327 <https://reviews.llvm.org/D76705#2029327>, @jonpa wrote:

> > Can we instead move the W->FP mnemonic conversion back into BinaryVRRa/TernaryVRRe instead, so that the OpKey for all these vector instructions consistently holds the FP-reg instruction name? The MemFold-pseudos would then have the same mnemonic as OpKey and MemKey.
> > 
> >   ... Ah, that still runs into the same name collisions with the MemFold pattern as you pointed out earlier. Sorry for missing that!
>
> Yeah, WFADB:ADBR would map to ADB, and not ADB_MemFoldPseudo which is what we need...
>
> > So if we use an extra modifier in the OpKey for those case, then we're basically back to your version of Apr 28, 7:15 PM, except possibly with the explicit mnemonics in the .td file.
>
> Ok, went back to that version plus added the explicit fp_mnemonic fields, which seems nice to me as they eliminate those ugly string substitutions.


Yes, this does look a lot more reasonable.  Thanks for going through all those iterations!

>> Hmm, talking about MemFold, I'm wondering about this:
> 
> ​>  // Fused multiply and add/sub need to have the same dst and accumulator reg.
> 
>> Given that this check is necessary, what's then the point of having a MemFold for those ternary instructions in the first place? Can't we then directly emit the target instruction?
> 
> I believe this follows the same pattern as the other MemFold cases: A WFMADB has four register operands, which all may be different virtual registers at this point. In the case where we find that DstReg and AccReg has already been allocated to the same physical register, we proceed with a MemFold pseudo. The register allocator may still however evict live ranges and reallocate these operands, which is why we can't go directly to MADB.

Ah, I see.  Makes sense.

See one final inline comment, otherwise this now looks good to me.



================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:4426
   let M6 = type;
+  let OpKey = mnemonic#tr1.op;
+  let OpKey = fp_mnemonic#"MemFold"#!subst("VR", "FP", !cast<string>(tr1.op));
----------------
That first line setting OpKey seems superfluous?


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

https://reviews.llvm.org/D76705