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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

This looks pretty good to me.   One thing that is odd is the extra always-zero parameter to the LoopVarLen insns.  This really should be removed.

As a suggestion that might remove a bit of the duplication between emitMemMemWrapper and emitMemMemLoopVarLen:  the (old) routine is currently called for both the Sequence and Loop insns, and it distinguishes between them based on the operand count.   However, for the LoopVarLen case, your patch now introduces a separate new routine.    I think it might be simpler to continue to use the same routine, and just internally distinguish between the cases based on whether the length operand is an immediate or a register.

Finally, I'm wondering if it wouldn't make sense to extend support for the variable-length case to the other block operations, now that we already have the EXRL_Pseudo infrastructure.  (I guess that could also be done in a separate patch.)

> Pardon the typo-fixes mixed in here (MMB -> MBB). Maybe pre-commit them?

Yes, please pre-commit those.



================
Comment at: llvm/lib/Target/SystemZ/SystemZAsmPrinter.h:14
 #include "SystemZMCInstLower.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/StackMaps.h"
----------------
The above clang-format check looks valid (includes should be kept sorted).


================
Comment at: llvm/lib/Target/SystemZ/SystemZAsmPrinter.h:29
   StackMaps SM;
+  typedef std::pair<MCSymbol*, MCInstBuilder> SymbInstPair;
+  std::vector<SymbInstPair> EXRL_Targets;
----------------
This as well.


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

https://reviews.llvm.org/D103865