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

[Sam Parker via Phabricator via llvm-commits]

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


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:646
+  default: break;
+  case Intrinsic::sqrt:
+  case Intrinsic::powi:
----------------
dmgreen wrote:
> There is a floating point sqrt instruction, at least for scalar fp. MVE does not have sqrt or div, but will expand into multiple scalar instructions.
cheers!


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:688
+  case Intrinsic::usub_sat:
+    return true; // FIXME Signed 32-bit could be selected to QADD/SUB.
+  }
----------------
dmgreen wrote:
> I think most intrinsics like these will be expanded, so not end up as functions (even if they are not very efficient).
That would make sense, I'll double check.


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:698
+                                          TTI::HardwareLoopInfo &HWLoopInfo) {
+  // FIXME: Low-overhead branches are only supported in the 'low-overhead branch'
+  // extension of v8.1-m.
----------------
dmgreen wrote:
> This is an extension to 8.1-m? Not just a base feature?
Section A1.4 'Armv8-M' variants reads to me that the LOB extension is optional on 8.1-M. The main extension doesn't appear to include LOB, but MVE-I does. I've raised this with Simon for the instruction support too.


================
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:
> Will getBackedgeTakenCount ever be nullptr (as opposed to something like SCEVCouldNotCompute below)
I'll check.


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

https://reviews.llvm.org/D62907