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

[Philip Reames via llvm-commits]

reames added a comment.

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.


http://reviews.llvm.org/D16986