[PATCH] D16986: [LICM] Don't assert on volatile accesses

[Nuno Lopes via llvm-commits]

nlopes added a comment.

In http://reviews.llvm.org/D16986#358822, @reames wrote:

> In http://reviews.llvm.org/D16986#358193, @nlopes wrote:
>
> > Just a quick comment on this bug. We can see it from two angles:
> >
> > 1. TBAA is just doing its job since it is exploiting the fact that 2 pointers with different types don't alias (the metadata has 2 different type branches).  So TBAA claims the pointers don't alias, while they do in practice (it's undefined behavior, though).  -- in this case LICM needs patching to account for this aggressiveness.
> > 2. Or we can say that TBAA shouldn't mess with volatile stuff since that's dangerous. In that case LICM is fine and TBAA needs patching.
> >
> >   I'm personally more favorable of 2), since I don't think it's worth to mess with volatile stuff, since it can be dangerous for applications and probably there isn't much performance to gain there.
>
>
> I disagree with your framing.  Unless I misunderstood the issue, this is a clear bug in AST regardless of what TBAA returns for aliasing information.  If we ended up with a volatile memory access being part of a alias set, that alias set *must* be marked volatile.  It's okay to have non-volatile accesses as part of the volatile set, but not the other way around.  If we required the isVolatile flag to be precise for all elements in the set, we'd essentially be requiring alias analysis to *always* get a precise no-alias result when it's possible to do so.  That's not an invariant we can uphold.


One of the alias set is not being marked as volatile because TBAA decides it cannot possibly alias with the volatile alias set (because of the UB).  But then store forwarding happens and LICM realizes (or doesn't) that two of the alias sets that were said to be disjoint by TBAA in fact alias.
It's not related with volatile information being or not being precise. I agree that we only aim for an over approximation.
The information returned by TBAA *is* correct.  I just believe it's too aggressive.

So I believe my analysis is correct. It's the fact that TBAA decides that two pointers don't alias and therefore only one of the sets gets marked as volatile.


http://reviews.llvm.org/D16986