[PATCH] D97729: [ARM] Improve WLS lowering

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/docs/LangRef.rst:15749
+
+The '``llvm.test.start.loop.iterations.*``' intrinsics are similar to the
+'``llvm.test.set.loop.iterations.*``' and '``llvm.start.loop.iterations.*``'
----------------
Sorry a bit of names bike shedding. But the context of this is that I found the code/diff difficult to read because these intrinsic names look so similar and found it difficult to remember their exact differences. So I was wondering if the 'start' in the name here could e.g. be 'startup', then that would be more consistent with the instructions/pseudo this maps to?

I actually need to read everything again here, but I was also wondering if we are still using `llvm.test.set.loop.iterations`?


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.h:134
+      WLS,          // Low-overhead loops, While Loop Start branch
+      WLSSETUP,     // Setup for the iteration count of a WLS
       LOOP_DEC,     // Really a part of LE, performs the sub
----------------
I think this could benefit from some further clarifications, i.e. what the exact difference is between WLS and WLSSETUP because that may not be so obvious from these comments. I think it is well explained in the description, so something along the lines of:

> Instead the t2WhileLoopSetup produces the value of LR (essentially acting as a lr = subs rn, 0), t2WhileLoopStart consumes that lr value (the Bcc).


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:5470
+// during MVETPAndVPTOptimisations.
+def t2WhileLoopSetup :
+  t2PseudoInst<(outs GPRlr:$lr), (ins rGPR:$elts), 4, IIC_Br, []>;
----------------
Do we need to say something about isTerminator or hasSideEffects too here?


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

https://reviews.llvm.org/D97729