[PATCH] D67948: [LV] Interleaving should not exceed estimated loop trip count.

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5212
 
   // Do not interleave loops with a relatively small trip count.
+  auto BestKnownTC = getSmallBestKnownTC(*PSE.getSE(), TheLoop);
----------------
hsaito wrote:
> small estimated or constant trip count
Done


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5213
   // Do not interleave loops with a relatively small trip count.
-  unsigned TC = PSE.getSE()->getSmallConstantTripCount(TheLoop);
-  if (TC > 1 && TC < TinyTripCountInterleaveThreshold)
+  auto BestKnownTC = getSmallBestKnownTC(*PSE.getSE(), TheLoop);
+  if (BestKnownTC && *BestKnownTC < TinyTripCountInterleaveThreshold)
----------------
hsaito wrote:
> This assumes constant trip count case is handled well by getSmallConstantMaxTripCount called from getSmallBestKnownTC ---- but if that is not the case, that would be a bug on the SCEV side. I traced a little bit but could not verify it myself as I'm not familiar with SCEV code. As such, I'm just pointing out a different SCEV function will be called as a result of this change.
Maybe I'm missing your concern but constant trip count case is processed first in getSmallBestKnownTC by a call to getSmallConstantTripCount. Thus I don't see any change for constant trip count case at all.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5265-5266
 
   // If the trip count is constant, limit the interleave count to be less than
   // the trip count divided by VF.
+  if (BestKnownTC) {
----------------
hsaito wrote:
> If trip count is expected to be small, limit the interleave count to be less than the trip count divided by VF
There is some ambiguity in using "small" through out the code. For getSmallBestKnownTC  "small" is if it fits 32-bit. For "if (BestKnownTC && *BestKnownTC < TinyTripCountInterleaveThreshold)" check "small" is what less than TinyTripCountInterleaveThreshold. Here "small" should refer to the meaning defined by getSmallBestKnownTC .
I think we better avoid using "small" one more time here to minimize the confusion.


================
Comment at: llvm/test/Transforms/LoopVectorize/interleave_short_tc.ll:65
+
+attributes #0 = { nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="none" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { argmemonly nounwind willreturn }
----------------
xbolva00 wrote:
> ebrevnov wrote:
> > reames wrote:
> > > Please remove unnecessary aspects of test.
> > Done
> +1
> 
> Just leave metadata which you really need for this test. (Lines 56-70 can be removed)
I tried to remove as much as I can but not all of them can be actually removed.


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

https://reviews.llvm.org/D67948