[PATCH] D93039: Introduce llvm.noalias.decl intrinsic

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added inline comments.


================
Comment at: llvm/lib/IR/Verifier.cpp:121
+    cl::desc("Ensure that llvm.experimental.noalias.scope.decl for identical "
+             "scopes are not dominating"));
+
----------------
fhahn wrote:
> jeroen.dobbelaere wrote:
> > fhahn wrote:
> > > jeroen.dobbelaere wrote:
> > > > nikic wrote:
> > > > > fhahn wrote:
> > > > > > jeroen.dobbelaere wrote:
> > > > > > > nikic wrote:
> > > > > > > > Can we flip this flag now, or are you aware of any further issues?
> > > > > > > I just did a test-suite run and a stage2 release build using an assertion build with the flag flipped, and both went fine. IMHO it makes sense to try it out...
> > > > > > > 
> > > > > > What's the impact on compile-time? If it is potentially expensive, it should probably be `true` for `EXPENSIVE_CHECKS` builds.
> > > > > I see no reason for it to be expensive at least.
> > > > @fhahn, @nikic, It's O(N log(N)) (sorting of N = number of scope declarations)
> > > > Next to that, it is roughly k * O((M-1)^2) * O(dominance checking) with M the the number of identical scope declarations, and k the number if different scopes.
> > > > In practice M should be pretty low (mostly 1) and O(N log(N)) should be the main part.
> > > > 
> > > > Is that considered to be too expensive ?
> > > > 
> > > > 
> > > IIUC we need to potentially check whether any `llvm.experimental.noalias.scope.decl` dominates any other `llvm.experimental.noalias.scope.decl`? 
> > > 
> > > 
> > > Is it possible to have a function with `N` basic blocks which do not dominate each other, each with `N` `llvm.experimental.noalias.scope.decl` for the same scope?
> > > Is it possible to have a function with `N` basic blocks which do not dominate each other, each with `N` `llvm.experimental.noalias.scope.decl` for the same scope?
> > It is allowed to construct such llvm-ir manually.
> > I am not aware on how to trigger this by an optimization pass, just based on declarations introduced by inlining.
> > My understanding is that most optimizations will not duplicate the intrinsic. The extra check is there to catch those optimizations that do and do it wrongly.
> > 
> >> In practice M should be pretty low (mostly 1) and O(N log(N)) should be the main part.
> >
> 
> I am not worried really worried about about the 'common' case here, it is unlikely to be noticeable in common C/C++ programs. But if there isn't anything preventing the worst case, (~`M` in the above being roughly the number of instructions in a function, excluding branches), then I think it would be better to have this as expensive check.
> 
> This might be overly cautious for 'sane' code, but unfortunately there are plenty of code-generators that like to generate highly unusual IR. For example, we surprisingly often get reports of builds taking 24+ hours, because someone tries to compile a file with 500k stores in a single basic block without running any IR store optimizations, which one could argue is not 'common'.
> 
> > It is allowed to construct such llvm-ir manually.
> >I am not aware on how to trigger this by an optimization pass, just based on declarations introduced by inlining.
> 
> Unfortunately LLVM IR & the verifier is used in many contexts, frontends are free to generate whatever IR they want and also are free to use whatever combination of optimizations (or no optimizations at all). Personally I am weary when it comes to assuming IR passed to LLVM is 'reasonable'.
> 
@fhahn Would D95335 be acceptable ?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93039