[HardwareLoops] Change order of SCEV expression construction for InitLoopCount.

[JanekvO]

Summary

Putting the +1 before the zero-extend will allow scalar evolution to fold the expression in some cases such as the one shown in PowerPC's shrink-wrap.ll test.

[samparker]

LGTM, thanks!

[samparker]

Out of curiosity, did you try performing the +1 in HardwareLoopInfo::isHardwareLoopCandidate instead? Looking back through the code, I think it would be more clear to rename 'ExitCount' to 'TripCount' and perform this addition outside of the pass.

[JanekvO]

In D91724#2404880, @samparker wrote:

Out of curiosity, did you try performing the +1 in HardwareLoopInfo::isHardwareLoopCandidate instead? Looking back through the code, I think it would be more clear to rename 'ExitCount' to 'TripCount' and perform this addition outside of the pass.

I hadn't considered the +1 in isHardwareLoopCandidate until you mentioned it. I can add these changes in this diff, but I wonder whether the zero-extend should also be moved to isHardwareLoopCandidate at this point. That way only the uses of ExitCount (or TripCount, after renaming) will remain in the pass (as far as I can see).

[samparker]

I wonder whether the zero-extend should also be moved to isHardwareLoopCandidate at this point.

Nice, sounds good to me.

[samparker]

llvm/lib/CodeGen/HardwareLoops.cpp

388 ↗

(On Diff #306152)

Could you also double check that this PointerTy check is necessary, it seems a little odd to me but I don't know much about the PPC implementation.

[JanekvO]

Rename ExitCount to TripCount and move SCAV expression construction outside of HardwareLoops pass into isHardwareLoopCandidate.

[JanekvO]

Diff against HEAD

[JanekvO]

Sorry, one more try to diff against the right branch

[JanekvO]

llvm/lib/CodeGen/HardwareLoops.cpp

388 ↗

(On Diff #306152)

I did a quick check and removed the PointerTy check and it seems all tests still run fine. However, I don't know enough about either PPC nor scalar evolution to confidently remove the check. Intuitively, it sounds unlikely for getExitCount to return a pointer type but I'm not sure.

[samparker]

Intuitively, it sounds unlikely for getExitCount to return a pointer type but I'm not sure.

Indeed, thanks for checking. LGTM, thanks.

[reames]

Glanced at this due to being CCed on pr51714.

This patch does not look correct on first glance. If backedge taken count is UINT_MAX for the narrower type, doing the increment in the narrow type produces zext(0) e.g. zero, whereas doing the zext(BTC)+1 does not. Unless there's something in the code which prevents BTC=UINT_MAX/TC=0, I suspect that's the cause of the miscompile being observed in that bug and this patch should be reverted.

[shchenz]

In D91724#2981131, @reames wrote:

Glanced at this due to being CCed on pr51714.

This patch does not look correct on first glance. If backedge taken count is UINT_MAX for the narrower type, doing the increment in the narrow type produces zext(0) e.g. zero, whereas doing the zext(BTC)+1 does not. Unless there's something in the code which prevents BTC=UINT_MAX/TC=0, I suspect that's the cause of the miscompile being observed in that bug and this patch should be reverted.

Thanks @reames
reverted in 34badc409cc452575c538c4b6449546adc38f121

[samparker]

Unless there's something in the code which prevents BTC=UINT_MAX/TC=0

And I thought there was!

Thanks both.

llvm/lib/Analysis/TargetTransformInfo.cpp

for (BasicBlock *BB : ExitingBlocks) {
     } else
       continue;
 
     // Note that this block may not be the loop latch block, even if the loop
     // has a latch block.
     ExitBlock = BB;
     TripCount = SE.getAddExpr(EC, SE.getOne(EC->getType()));
 
-    if (!EC->getType()->isPointerTy() &&
-        EC->getType() != CountType)
+    if (!EC->getType()->isPointerTy() && EC->getType() != CountType)
       TripCount = SE.getZeroExtendExpr(TripCount, CountType);
 
     break;
   }
 
   if (!ExitBlock)
     return false;
   return true;