david-arm added a comment.
Hi @junparser, the patch looks sensible to me! I just had a couple of minor
comments if that's ok.
================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:658
+ return IC.replaceInstUsesWith(II, VScale);
+ } else if (Pattern >= AArch64SVEPredPattern::vl1 &&
+ Pattern <= AArch64SVEPredPattern::vl8 && NumElts >= Pattern) {
----------------
nit: Do you need to fix the clang-tidy warning here?
================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:662
+ return IC.replaceInstUsesWith(II, StepVal);
+ } else if (Pattern == AArch64SVEPredPattern::vl16 && NumElts == 16) {
+ Constant *StepVal = ConstantInt::get(II.getType(), NumElts);
----------------
Could you potentially fold these two cases into one somehow? Maybe with a
switch-case statement? I'm just imagining a situation where we might want other
patterns too like vl32, vl64, etc.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104852/new/
https://reviews.llvm.org/D104852
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
