[PATCH] D72178: [DA] Delinearization of fixed-size multi-dimensional arrays

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a subscriber: pcc.
Meinersbur added inline comments.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3360-3361
 
-  // Below code mimics the code in Delinearization.cpp
-  const SCEV *SrcAccessFn =
-    SE->getSCEVAtScope(SrcPtr, SrcLoop);
-  const SCEV *DstAccessFn =
-    SE->getSCEVAtScope(DstPtr, DstLoop);
+  if (!DisableDelinearizationChecks)
+    return false;
 
----------------
bmahjour wrote:
> Meinersbur wrote:
> > Please add a comment why this is bailing out here.
> > 
> > We could also check for GEP's [[ https://llvm.org/docs/LangRef.html#getelementptr-instruction | inrange ]] modifier and require it unless `DisableDelinearizationChecks` is set.
> > We could also check for GEP's inrange modifier and require it unless DisableDelinearizationChecks is set.
> 
> We could, however I'd suggest we consider that as a separate patch. There are some peculiarities with GEP's inrange that I need to understand better. In particular, it's not clear to me why the syntax allows `inrange` to appear before multiple indexes, but the `getInRangeIndex()` API only allows a single index to be retrieved. 
> 
> If I understand this conversation https://reviews.llvm.org/D22793?id=65626#inline-194586 correctly, then we can only try to delinearize GEPs when
> `SrcGEP->getInRangeIndex().getValue() == SrcGEP->getNumIndices()`. Is that true?
> We could, however I'd suggest we consider that as a separate patch. 

Agreed. Just something to keep in mind when renaming `-da-disable-delinearization-checks` that might not totally disable non-linear GEPs unless set.

> the getInRangeIndex() API only allows a single index to be retrieved.

Yes, and it contradicts the LLVM reference for `inrange`: https://llvm.org/docs/LangRef.html#id231 . It also seems to be allowed for constant expressions currently.

> <result> = getelementptr inbounds <ty>, <ty>* <ptrval>{, [inrange] <ty> <idx>}*
> ... if the load or store would access memory outside of the bounds **of the element selected by the index marked as inrange**. 


For reference, here is the inline comment from @pcc:

> No more than one. The rationale is that inrange on a given index will be at least as restrictive as inrange on any earlier index, so there's no point in allowing more than one. I'll document this in the langref.

This seems not to have been documented. However, there are other motivations than @pcc had in mind ([[ http://lists.llvm.org/pipermail/llvm-dev/2016-July/102472.html | vtables ]]) and some may allow non-inrange indices after an inrange one. Furthermore, I think it is problematic if we'd implicitly interpret all subscripts after an inrange subscript as well.







Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72178