[PATCH] D62907: [ARM] Implement TTI::isHardwareLoopProfitable

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Looks like a good start for these loloops. I'm guessing we can always extend MaybeCall if we find more cases that end up as calls. (Along with the difficulties of tail predication!)



================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:704
+  // For now, for simplicity, only support loops with one exit block.
+  if (!L->getExitBlock() || !SE.getBackedgeTakenCount(L))
+    return false;
----------------
samparker wrote:
> dmgreen wrote:
> > Will getBackedgeTakenCount ever be nullptr (as opposed to something like SCEVCouldNotCompute below)
> I'll check.
I also see the blurb for getBackedgeTakenCount talking about calling hasLoopInvariantBackedgeTakenCount, but many passes don't seem to check that. Maybe worth adding a check in place of !SE.getBackedgeTakenCount(L).


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:656
+  case Intrinsic::sqrt:
+    if (F->getReturnType()->isVectorTy())
+      return true;
----------------
I think that even a vector type should become scalar sqrts. Same for abs and friends. So they wouldn't remain as vector operations (which may effect tail prediction), but wouldn't become calls, unless the scalar version will be calls.


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:723
+  auto MaybeCall = [this](Instruction &I) {
+
+    // Check if an intrinsic will be lowered to a call and assume that any
----------------
I would remove this line :)


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:775
+    if (Ty->isDoubleTy() && !ST->hasFP64())
+      return false;
+
----------------
Should this one be to return true?


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

https://reviews.llvm.org/D62907