[PATCH] D31885: Remove TBAA information from LValues representing union members

[Daniel Berlin via Phabricator via cfe-commits]

dberlin added a comment.

In https://reviews.llvm.org/D31885#730936, @rjmccall wrote:

> In https://reviews.llvm.org/D31885#730920, @dberlin wrote:
>
> > Just so i understand: Ignoring everything else (we can't actually make likelyalias work, i think the code in the bugs makes that very clear),
>
>
> None of the code in the bugs I've seen makes that very clear, but perhaps I just missed the compelling example.
>
> > you also believe we should effectively pessimize every other language that generates correct TBAA info for LLVM  and will now no longer get optimized well because we've decided not to have clang emit TBAA metadata only in the cases where  it actually wants TBAA applied?
>
> Are you under the impression that I'm proposing something new and that TBAA does not currently defer to BasicAA?


Well, actually, chandler changed how it works, so it doesn't actually work quite the way it used to but it has the same effect ;)
(It used to be explicit deferral, it is not explicit, neither depends on the other any more.  It just happens to be how the default AA pipeline is ordered right now)
I'm quite aware of the machinations of AA in LLVM :)

I'm saying "this approach does not work well now" (ie see bugs) , and i don't think continuing to try to make the contract even *more* incestuous is better than trying to make it *less*.

I see literally no advantage to doing so, and plenty of disadvantage (pessimization of other languages, continuation of a pipeline ordering that requires overrides, etc)

IE this is a thing we should be fixing, over time, to be a cleaner and better design, that does not make clang special here, and fixes these bugs.
Others generate tbaa metadata where they want it, and avoid it where they want basicaa to make a decision.

I have trouble seeing, why, if folks are willing to do that work, one would be opposed to it.
You seem to claim better optimization opportunity.
I ran numbers.
Across all things in llvm's benchmark suite, disabling unions entirely for tbaa causes no regressions on x86 (I could try more platforms if you like).

I also  ran it across 50 third party packages and spec2006 that use strict aliasing (These are just those things i could quickly test that had perf benchmarks)

If it really caused any real perf loss, we could always define metadata to say these accesses are noalias if we can't prove they are mustalias.
We could even use that metadata to explicitly try to have basicaa try much harder than it does now to prove must-aliasing (for example, ignore depth limits), since it would likely be too expensive to try it for everything.

as an aside, I do not believe you could reasonably apply this to unions  because it's not possible to tell people what to do differently when it's *llvm ir* that has the issue.

IE i can't reasonably tell users "please stop doing things that don't generate explicit enough IR".
Especially when those answers change with inlining decisions, etc.

If the frontend did it, i would likely have something reasonably to tell them.


Repository:
  rL LLVM

https://reviews.llvm.org/D31885