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

[Sam Parker via Phabricator via llvm-commits]

samparker marked 3 inline comments as done.
samparker added inline comments.


================
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;
----------------
dmgreen wrote:
> 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).
I saw that getBackedgeTakenCount appears to always return some sort of value, but I had certainly missed the LoopInvariant restriction - which seems arbitrary and unnecessary to me! But I'll follow the good practise.


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:656
+  case Intrinsic::sqrt:
+    if (F->getReturnType()->isVectorTy())
+      return true;
----------------
dmgreen wrote:
> 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.
Okay, I'll query TLI here instead and check for Expand or LibCall.


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:775
+    if (Ty->isDoubleTy() && !ST->hasFP64())
+      return false;
+
----------------
dmgreen wrote:
> Should this one be to return true?
bah! yep, probably should add some double tests...


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

https://reviews.llvm.org/D62907