[PATCH] D147117: [SCEV] When computing trip count, only zext if necessary

[Joshua Cao via Phabricator via llvm-commits]

caojoshua added inline comments.


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:337
   if (auto ConstantCost = dyn_cast<SCEVConstant>(RefCost))
-    return ConstantCost->getValue()->getSExtValue();
+    return ConstantCost->getValue()->getZExtValue();
 
----------------
caojoshua wrote:
> nikic wrote:
> > caojoshua wrote:
> > > nikic wrote:
> > > > The changes to LoopCacheAnalysis should be split out into a separate patch. I'm also wondering why this changes some but not all of the AnyExtends...
> > > The places where I changed sext->zext are SCEVs that are computed from TripCount. In this case, RefCost is set to TripCount in the else block above.
> > > 
> > > I thought it made sense to have a single patch for these two changes. It would be harder to motivate the changes if I only submitted a patch for LoopCacheAnalysis. And the whole patch is small overall, so its not hard to read.
> > But aren't the two AnyExtends in the `else` block above also on trip counts?
> You're right. I think its just lack of test coverage in LoopCacheAnalysis / LoopInterchange, and that we should be using `zext` in those cases.
> 
> These cases don't come up very often. It's specifically when `BackedgeTakenCount = false`, or when there is a dead loop. The code we are talking about only applies when there are >=3 subscripts, and there probably just is not a test for it yet.
> 
> When I get the chance, I'll see if I can create a breaking test.
Recall the issue only occurs when `BackedgeTakenCount = False`, resulting in `TripCount = True`. For 

```
      Type *WiderType = SE.getWiderType(RefCost->getType(), TripCount->getType());
```

The WiderType is always going to just be 1. And the loop just multiplies `True x True = True`. We end up returning `zext(True) = 1`. If I didn't make the change here, we would have return `sext(True) = -1`.

I'm gonna push a change to make those zero extends anyway. The tests still pass. I think zero extends make sense here because the returned RefCost is always positive, or -1 if the cost is invalid.

For reference, I pushed a [test](https://github.com/llvm/llvm-project/commit/a400c6ac03371b5b888d35e72187210303b1877f) that helped me debug this issue. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147117