[PATCH] D17080: [LAA] Allow more run-time alias checks by coercing pointer expressions to AddRecExprs

silviu.baranga@arm.com via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 21 02:46:17 PDT 2017


sbaranga added a comment.

Sorry for not replying earlier. It looks like there are only minor changes left? I plan to push an update after this gets unblocked.

It's also a little bit odd that I haven't been able to get a definitive review for this. It would be good to know if there's something fundamentally wrong with the approach (at least we could start thinking about workarounds).



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:643
   int64_t Stride = getPtrStride(PSE, Ptr, L, Strides);
-  return Stride == 1;
+  if (Stride == 1 || PSE.hasNoOverflow(Ptr, SCEVWrapPredicate::IncrementNUSW))
+    return true;
----------------
Ayal wrote:
> The tests added below check this other added condition, namely PSE.hasNoOverflow(), right?
Correct.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:748
+      NeedsCheck = (NumWritePtrChecks >= 2 ||
+                    (NumReadPtrChecks >= 1 && NumWritePtrChecks >= 1));
+
----------------
Ayal wrote:
> This goes back to the original code:
> it may be slightly easier to always bump ++NumTotalPtrChecks instead of ++NumReadPtrChecks and set
> 
> ```
> NeedsCheck = (NumTotalPtrChecks >= 2 && NumWritePtrChecks >= 1);
> ```
> 
> 
> But not sure I follow the original logic here; it's equivalent to doing:
> 
> ```
> bool NeedsCheck = (NumTotalPtrChecks >= 2 && NumWritePtrChecks >= 1);
> if (IsDepCheckNeeded && CanDoAliasSetRT && RunningDepId == 2)
>   NeedsCheck = false;
> ```
> 
> Having CanDoAliasSetRT && RunningDepId == 2 implies that NumTotalPtrChecks == 2, but why set NeedsCheck to false when this happens and IsDepCheckNeeded? What if NumWritePtrChecks >= 1?
> 
> 
> May want to rename NeedsCheck >> NeedsAliasSetRTCheck, analogous to the CanDo's.
I've tried to preserve the original logic, so if there are any issues with the original logic I've also pulled them in. The only difference here being that we're doing the same logic per alias set.

It is possible that the logic can be simplified.

RunningDepId == 2 means that we only have one dependence set. There a comment above " But there is no need to checks if there is only one dependence set for this alias set.". If I remember correctly this covers cases such as:

for (int i = 0; i < n; ++i)
  a[i] = a[i] + 1;

where we don't need any checks.

For renaming NeedsCheck >> NeedsAliasSetRTCheck: I have no objections.



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:792
         DEBUG(dbgs() << "LAA: Runtime check would require comparison between"
                        " different address spaces\n");
         return false;
----------------
Ayal wrote:
> ok, but what if !NeedRTCheck?
If !NeedRTCheck I don't think we need to do this check.

However, the old code was doing it, so I left it in.


https://reviews.llvm.org/D17080





More information about the llvm-commits mailing list