[PATCH] D103865: [SystemZ] Generate XC loop for memset 0 of variable length.

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

In D103865#2848831 <https://reviews.llvm.org/D103865#2848831>, @jonpa wrote:

> 



> ah, of course - only one label emitted now. They by this get sorted per the operands in the comparator for the std::map object, but a little detail then is that the labels now get emitted in any order (non-sorted) - but I suppose that's ok, right?

That shouldn't matter.

> I started out with trying to "assemble" by hand per your suggestion, but it seemed like quite some work, especially considering that we also have to be able to print it as text.

I was thinking of just using emitInt16 for the three words, that will show as .word in the textual assembler output, which may not be pretty but is certainly correct.  (You could in addition use an emitRawComment to print a textual representation to make the file more readable).

> So I tried instead with adding back the GenericSubtarget, which was simple enough.

See the comments inline on that option.

In the meantime I thought of yet another option: the instructions used via EXRL are really part of the function containing the EXRL, and should therefore be emitted using the same Subtarget that is in effect for that function (this is not *currently* a problem, but it might be a potential issue if we want to use an EXRL target instruction that is only available in some ISA levels).

To correctly model this, one option might be to not have a single EXRLT2SymMap, but one per Subtarget, and sort the target instructions into the appropriate subtarget for the current function as they're added to the map.  (Or, even simpler, just make the Subtarget part of the "key" for the map.)

In the common case where all functions use the same Subtarget, this would actually lead to the same result as now.

> This then eliminates another 3786 XC targets: there are no duplicates in any file, and at most I see 13 different XC targets in a single file :-)

Excellent!



================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:7895
     // The prefetch is used only for MVC.  The JLH is used only for CLC.
-    MBB = LoopMBB;
-
-    BuildMI(MBB, DL, TII->get(SystemZ::PHI), ThisDestReg)
+    BuildMI(LoopMBB, DL, TII->get(SystemZ::PHI), ThisDestReg)
       .addReg(StartDestReg).addMBB(StartMBB)
----------------
All these changes seem unnecessary now.


================
Comment at: llvm/lib/Target/SystemZ/SystemZSelectionDAGInfo.cpp:142
+  else { // Variable length
+    if (CByte && CByte->getZExtValue() == 0) {
+      // Handle the special case of a variable length memset of 0 with XC.
----------------
The above suggestion actually makes sense, we don't need a else here.


================
Comment at: llvm/lib/Target/SystemZ/SystemZTargetMachine.h:48
   // attributes of each function.
   const SystemZSubtarget *getSubtargetImpl() const = delete;
 
----------------
So your new "getGenericSubtarget" does exactly the same that the no-argument getSubtargetImpl used to do.  This is a bit odd, in particular given the comment here.

If we do want to go that route, I think we should actually use the getSubtargetImpl name, but change the comment to explain what the "default" subtarget is, and what we use it for.


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

https://reviews.llvm.org/D103865