[PATCH] D25557: [LAA] Collect pointers with unknown bounds

[Evgeny Astigeevich via llvm-commits]

eastig added inline comments.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:506
+  /// \brief Set of pointers with unknown bounds.
+  SmallVector<Value *, 4> PtrsWithUnknownBounds;
 };
----------------
I think this should be changed to 

```
SmallVector<TrackingVH<Value>, 4> PtrsWithUnknownBounds;
```

because PtrsWithUnknownBounds will live after the pass.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:661
         DEBUG(dbgs() << "LAA: Can't find bounds for ptr:" << *Ptr << '\n');
+        RtCheck.pointerWithUnknownBounds(Ptr);
         CanDoRT = false;
----------------
anemet wrote:
> pointer is not a verb, you need to use add or something
I'll change


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:718
 
-  bool CanDoRTIfNeeded = !NeedRTCheck || CanDoRT;
-  if (!CanDoRTIfNeeded)
-    RtCheck.reset();
-  return CanDoRTIfNeeded;
+  return !NeedRTCheck || CanDoRT;
 }
----------------
anemet wrote:
> eastig wrote:
> > ashutosh.nema wrote:
> > > eastig wrote:
> > > > ashutosh.nema wrote:
> > > > > Earlier when “NeedRTCheck” is true & LAA can’t find the bounds of pointer it used to reset the “Need” field of RuntimePointerChecking, but with this change it can remains set. Is this expected change ?
> > > > The old behaviour is to reset RtCheck if we can not do RT checks when they are needed. Resetting means to make RtCheck Pointers and Checks empty. When they are empty it doesn't matter what a value Need has. We can do nothing with such RtCheck. But when Pointers is not empty the value of Need is important. According its documentation Need indicates if we need to add the runtime check. So when it's true RT checks might be created. They are not created if there are pointers with unknown bounds. In such case it's a user responsibility to create checks. The user can analyze Pointers and PtrsWithUnknownBounds and create needed checks.
> > > > I hope this explains why Need must be preserved.
> > > I’m worried about cases where decisions are made based on “Need” flag, for example vectorizer. Earlier when this flag is true, vectorizer expects complete runtime check from LAA but with this change it may not be the same case, rather it will be expected from vectorizer to generate the complete check.
> > I've looked how "Need" is used. It is used according to its meaning: RT checks need to be added. The vectorizer uses LAI->canVectorizeMemory() to check if a loop can be vectorized. canVectorizeMemory()  returns the value of CanVecMem which is set to false if CanDoRTIfNeeded is false (Need is true but CanDoRT is false). So in places where the vectorizer expects complete runtime checks they are complete because CanVecMem guarantees this.
> > 
> > I can update documentation of "Need" to mention that when it is true it does not mean it can do RT check. This is what it's written in comments to AccessAnalysis::canCheckPtrAtRT. Also when "Need" is calculated both pointers with known bounds and pointers with unknown bounds are taken into account.
> > 
> > 
> For the record, I've looked at the other clients as well (LLE, LDist).  They don't check Need.  They look for specific type of dependences but the set of dependences is empty unless we were also able to correctly populate the set of memchecks (CanDoRTIfNeeded).
> 
> Also you shouldn't add a new feature (PtrsWithUnknownBounds) and changing the semantics of an existing API (whether RtChecks is clear or not) in the same patch.
> 
> You should probably do the latter first and explain how it effects existing clients (LV, LLE, LDist).
> 
> Moving on to PtrsWithUnknownBounds, right now the generic client code to LAA is something like this:
> 
> ```
>  LAI = LAA->getInfo(L);
>  Deps = LAI->getDepChecker()->getDependences();
>  if (!Deps)
>    return; // couldn't analyze the loop
>  
>  // we have a set of dependences that are true if the all the run-time checks pass
>  RTC = LAI->getRuntimePointerChecking();
>  Checks = RTC->getChecks();
>  if (RTC.Need && Checks.empty()
>    return; // couldn't generate RT checks
> ```
> (For this latter check, I would actually prefer if getChecks would return null if we could't generate the checks.)
> 
> It would be good if you could describe how this would change with PtrsWithUnknownBounds if at all.
> 
> Also you shouldn't add a new feature (PtrsWithUnknownBounds) and changing the semantics of an existing API (whether RtChecks is clear or 
> not) in the same patch.

Do you suggest to split the patch into:
# Remove the call of reset.
# Add PtrsWithUnknownBounds.

> It would be good if you could describe how this would change with PtrsWithUnknownBounds if at all.

I think the current use cases won't be affected. They don't make a decision based only on "Need". They use flags which are based on Need and CanDoRt.





https://reviews.llvm.org/D25557