[PATCH] D104852: [AArch64][SVEIntrinsicOpts] Convect cntb/h/w/d to vscale intrinsic or constant.

[David Sherwood via Phabricator via cfe-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:671
+  case AArch64SVEPredPattern::vl8:
+    return NumElts >= Pattern
+               ? Optional<Instruction *>(IC.replaceInstUsesWith(
----------------
I was actually wondering if we could commonise this code somehow. Perhaps by setting a MinNumElts variable in the case statements, i.e.

  unsigned MinNumElts;
  ...
  case AArch64SVEPredPattern::vl8:
    MinNumElts = Pattern;
    break;
  case AArch64SVEPredPattern::vl16:
    MinNumElts = 16;
    break;
  }

  if (NumElts < MinNumElts) return None;

  return Optional<Instruction *>(IC.replaceInstUsesWith(
                               II, ConstantInt::get(II.getType(), NumElts)));


================
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);
----------------
junparser wrote:
> david-arm wrote:
> > 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.
> > 
> There is no other special pattern except vl16. But I do think switch-case is more straightforward
OK, thanks for making this a switch statement. I was just thinking that in the developer manual we say we also support vl1-vl256 so at some point we may add more enums in LLVM too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104852