[PATCH] D87457: [ARM][TTI] Prevents constants in a min/max pattern from being hoisted when in a loop

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks. Looks good.



================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:328
 
+  // Ensures constants of min(max()) or max(min()) patterns don't get hoisted
+  if (Inst && (ST->hasV6Ops() || ST->isThumb2()) &&
----------------
Can you add something about matching ssat in this comment. You could consider pulling it out into a separate function too.


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:329
+  // Ensures constants of min(max()) or max(min()) patterns don't get hoisted
+  if (Inst && (ST->hasV6Ops() || ST->isThumb2()) &&
+      Ty->getIntegerBitWidth() <= 32) {
----------------
The condition might be better as `((ST->hasV6Ops() && !ST->isThumb()) || ST->isThumb2())`, as there are some cpu's (like the cortex-m0) which have V6Ops but not A32 or SSat.


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:346-347
+              matchSelectPattern(MinInst, MinLHS, MinRHS).Flavor;
+          if (PatternMatch::match(MinRHS, PatternMatch::m_ConstantInt(MinC)) &&
+              MinC->getUniqueInteger() == ((-Imm) - 1) && MinSPF == SPF_SMIN)
+            return true;
----------------
If this checks for MinSPF == SPF_SMIN first, then in case that is false we needn't look at the minRHS.

I think getUniqueInteger can just be getValue too, if we know it's a ConstantInt?


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

https://reviews.llvm.org/D87457