[PATCH] D97729: [ARM] Improve WLS lowering

[Dave Green via Phabricator via llvm-commits]

dmgreen 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.*``'
----------------
SjoerdMeijer wrote:
> 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`?
Yeah. I didn't think t2WhileLoopStartLR was particularly imaginative either.

For these hardware loop intrinsics, we either use:
llvm.set.loop.iterations and llvm.test.set.loop.iterations
or
llvm.start.loop.iterations and llvm.test.start.loop.iterations
depending on whether the backend requests UsePHICounter. We at Arm request that to make sure the values are all glued together properly, but other backends needn't. So this was attempting to be consistent with the llvm.start.loop.iterations that was changed back in D89881.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:5470
+// during MVETPAndVPTOptimisations.
+def t2WhileLoopSetup :
+  t2PseudoInst<(outs GPRlr:$lr), (ins rGPR:$elts), 4, IIC_Br, []>;
----------------
SjoerdMeijer wrote:
> Do we need to say something about isTerminator or hasSideEffects too here?
Do you mean as a comment? Or their attribute values? The defaults should be fine for the attributes I think. I've tried to expand the comment for all these too.


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

https://reviews.llvm.org/D97729