[PATCH] D109676: [HardwareLoops] put +1 for loop count before zero extension

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I posted a SCEV cleanup change inspired by this review.  It shouldn't block this one.  See https://reviews.llvm.org/D110587



================
Comment at: llvm/lib/CodeGen/HardwareLoops.cpp:386
   // the loop header, so also check if it has a single predecessor.
-  if (SE.isLoopEntryGuardedByCond(L, ICmpInst::ICMP_NE, ExitCount,
-                                  SE.getZero(ExitCount->getType()))) {
+  if (SE.isLoopEntryGuardedByCond(L, ICmpInst::ICMP_NE, TripCount,
+                                  SE.getZero(TripCount->getType()))) {
----------------
samparker wrote:
> shchenz wrote:
> > reames wrote:
> > > Hm, hadn't noticed this before.
> > > 
> > > An actual trip count is never zero.  This appears to have been trying to handle the overflow in BTC to TC conversion.  I haven't looked at this to see what it's doing and why, but this is likely dead code now and you should understand why this was insufficient previously.  
> > I don't think it is used to detect overflow in the original patch. It should be used to detect the possibility that the loop count can be zero. If it can not be a zero(for example, explicitly check the loop count and constant zero in the IR), some targets like ARM can use 'test and set' form loop count intrinsic, so that we can delete the explicitly check instruction in the IR. See one example in `test/Transforms/HardwareLoops/loop-guards.ll` `@test6()`. After the change, instruction `%cmp = icmp ne i32 %N, 0` has no user.
> > 
> > For the previous bug in https://reviews.llvm.org/D91724, this check does not help. On PowerPC, there is no `test and set` form loop count intrinsic, so no matter what the result of `isLoopEntryGuardedByCond` is, `UseLoopGuard` is always false. And the loop count value may still be changed from umax_32 + 1(if we extend to u64) to 0 after D91724.
> > 
> > At last, yes, after this patch, this check should not be done two times. I have updated the patch.
> There's a comment of line 417 about this. I was trying to familiarise, again, with this code the other day and it confused me... SCEV didn't seem to help much here and so we use pattern matching in CanGenerateTest to achieve what we want. But yes, looks like dead code now.
I'm not following this comment.  

My best guess is that you believe that the trip count of a loop which is not entered is zero.  This is not necessarily true.  A trip count (as returned by SCEV) is the number of times the loop header is executed if the loop is reached.  As such, a trip count is always 1 or greater.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109676