[PATCH] D65884: [ARM] MVE Tail Predication

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

If I am not mistaken, there is no test with a nested-loop. Would probably be good to test that too.



================
Comment at: lib/Target/ARM/MVETailPredication.cpp:301
+        unsigned ElementWidth = VecTy->getScalarSizeInBits();
+        if (Lanes * ElementWidth != 128 || Lanes == 128)
+          return false;
----------------
SjoerdMeijer wrote:
> A few nits I forgot to mention in my previous message:
> 
> Replace constant 128 with `TTI.getRegisterBitWidth(true)`?
> 
> And I don't think I understand the `Lanes == 128` part.
thanks for adding the comment, I still think you can use TTI.getRegisterBitWidth(true) here


================
Comment at: lib/Target/ARM/MVETailPredication.cpp:177
+  auto &SE = getAnalysis<ScalarEvolutionWrapperPass>().getSE();
+  TPCandidate Candidate(L);
+  bool Changed = Candidate.TryConvert(Setup->getArgOperand(0), SE);
----------------
For me, personally, creating a TPCandidate class doesn't add an abstraction that is really useful or clearer as literally the only thing it captures is 1 list, i.e. `MaskedInsts`, and we don't have multiple candidates and the lifetime of this object is extremely short. So, a simplification/clean-up in my opinion, is to get rid of the class and then we just have 3 local helper functions, and one variable that we pass thought. This is probably a nit, and/or a style preference.


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

https://reviews.llvm.org/D65884