[PATCH] D95335: [Verifier] enable and limit llvm.experimental.noalias.scope.decl dominance checking

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/IR/Verifier.cpp:5606
     } while (ItNext != NoAliasScopeDecls.end() &&
              GetScope(*ItNext) == CurScope);
 
----------------
nikic wrote:
> I wouldn't bother with the EXPENSIVE_CHECKS part and add a hardcoded limit of ItNext - ItCurrent < 32 or so here (only the case where there are many declares for the same scope is problematic).
> 
> It would be better to instead sort by DomTree DFS In number here and thus avoid a quadratic number of checks, but I don't think it's really worthwhile (the concern is a purely theoretical one).
> I wouldn't bother with the EXPENSIVE_CHECKS part and add a hardcoded limit of ItNext - ItCurrent < 32 or so here (only the case where there are many declares for the same scope is problematic).


SGTM

> It would be better to instead sort by DomTree DFS In number here and thus avoid a quadratic number of checks, but I don't think it's really worthwhile (the concern is a purely theoretical one).

I was thinking about suggesting this was well, but it's probably more trouble than it is worth.


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

https://reviews.llvm.org/D95335