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

kparzysz abandoned this revision. kparzysz added a comment. This review is replaced by https://reviews.llvm.org/D33328. Repository: rL LLVM https://reviews.llvm.org/D31885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

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

kparzysz added a comment. I posted https://reviews.llvm.org/D33284 for the change from AlignmentSource to LValueBaseInfo. Repository: rL LLVM https://reviews.llvm.org/D31885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#755456, @kparzysz wrote: > In https://reviews.llvm.org/D31885#751164, @rjmccall wrote: > > > The right fix is probably just to make sure that EmitLValueForField doesn't > > add TBAA information when the base LValue doesn't have it. Th

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

kparzysz added a comment. In https://reviews.llvm.org/D31885#751164, @rjmccall wrote: > The right fix is probably just to make sure that EmitLValueForField doesn't > add TBAA information when the base LValue doesn't have it. That will also > fix problems with recursive member access where the

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

rjmccall added a comment. The right fix is probably just to make sure that EmitLValueForField doesn't add TBAA information when the base LValue doesn't have it. That will also fix problems with recursive member access where the base is may_alias. Repository: rL LLVM https://reviews.llvm.or

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

kparzysz added a comment. I'm not intimately familiar with clang, so if you know about a better place for doing this, please let me know. Repository: rL LLVM https://reviews.llvm.org/D31885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

kparzysz added a comment. In https://reviews.llvm.org/D31885#748889, @rjmccall wrote: > Sounds to me like we should just not apply struct-path TBAA data that runs > through a union field because either LLVM's representation can't handle it or > Clang isn't generating the representation right.

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#748873, @kparzysz wrote: > Ping. > > What's the next step here? Sounds to me like we should just not apply struct-path TBAA data that runs through a union field because either LLVM's representation can't handle it or Clang isn't gen

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

kparzysz added a comment. Ping. What's the next step here? Repository: rL LLVM https://reviews.llvm.org/D31885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#731306, @hfinkel wrote: > In https://reviews.llvm.org/D31885#730853, @hfinkel wrote: > > > > > > ... > > > > > > >> (And the nonsensicality of the standard very much continues. The code that > >> we've all agreed is obviously being m

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

hfinkel added a comment. In https://reviews.llvm.org/D31885#730853, @hfinkel wrote: > ... > > >> (And the nonsensicality of the standard very much continues. The code that >> we've all agreed is obviously being miscompiled here is still a strict >> violation of the "improved" union rules i

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#730958, @dberlin wrote: > 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 > >

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

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)

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

rjmccall added a comment. 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, b

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

dberlin added a comment. In https://reviews.llvm.org/D31885#730909, @rjmccall wrote: > In https://reviews.llvm.org/D31885#730853, @hfinkel wrote: > > > > There was a deliberate decision to make TBAA conservative about type > > > punning in LLVM because, in practice, the strict form of TBAA you t

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#730853, @hfinkel wrote: > > There was a deliberate decision to make TBAA conservative about type > > punning in LLVM because, in practice, the strict form of TBAA you think we > > should follow has proven to be a failed optimization p

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

dberlin added a comment. > Your proposal to we simply drop TBAA from all accesses related to unions is > extremely conservative. It means that an access through a union has to be > treated as potentially aliasing every other visible access, at least in terms > of their types. That level of

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#730799, @dberlin wrote: > In https://reviews.llvm.org/D31885#730766, @rjmccall wrote: > > > In https://reviews.llvm.org/D31885#730539, @dberlin wrote: > > > > > In https://reviews.llvm.org/D31885#730072, @rjmccall wrote: > > > > > > > T

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

hfinkel added a comment. In https://reviews.llvm.org/D31885#730728, @kparzysz wrote: > In https://reviews.llvm.org/D31885#730038, @hfinkel wrote: > > > I'm somewhat concerned that this patch is quadratic in the AST. > > > I'd be happy to address this, but I'm not sure how. Memoizing results coul

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

hfinkel added a comment. > There was a deliberate decision to make TBAA conservative about type punning > in LLVM because, in practice, the strict form of TBAA you think we should > follow has proven to be a failed optimization paradigm: it just leads users > to globally disable TBAA, which is

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

dberlin added a comment. In https://reviews.llvm.org/D31885#730766, @rjmccall wrote: > In https://reviews.llvm.org/D31885#730539, @dberlin wrote: > > > In https://reviews.llvm.org/D31885#730072, @rjmccall wrote: > > > > > Thanks for CC'ing me. There are two problems here. > > > > > > The second

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

kparzysz updated this revision to Diff 95780. kparzysz added a comment. Memoize known results of checking for union access. Repository: rL LLVM https://reviews.llvm.org/D31885 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CodeGenFunction.cpp lib/CodeGen/CodeGenFunction.h test/CodeGen/uni

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#730539, @dberlin wrote: > In https://reviews.llvm.org/D31885#730072, @rjmccall wrote: > > > Thanks for CC'ing me. There are two problems here. > > > > The second is that our alias-analysis philosophy says that the fact that > > two ac

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

kparzysz added a comment. In https://reviews.llvm.org/D31885#730038, @hfinkel wrote: > I'm somewhat concerned that this patch is quadratic in the AST. I'd be happy to address this, but I'm not sure how. Memoizing results could be one way, but don't know if that's acceptable. This location in

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

dberlin added a comment. In https://reviews.llvm.org/D31885#730072, @rjmccall wrote: > Thanks for CC'ing me. There are two problems here. > > The second is that our alias-analysis philosophy says that the fact that two > accesses "sufficiently obviously" can alias should always override TBAA.

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

rjmccall added a comment. Thanks for CC'ing me. There are two problems here. The first is that vectors are aggregates which contain values of their element type. (And honestly, we may need to be more permissive than this because people are pretty lax about the element type in a vector until i

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

hfinkel added a comment. To be clear, I'm find with disabling union tbaa until we can fix things for real. I'm somewhat concerned that this patch is quadratic in the AST. FWIW, I tried just setting TBAAPath = true in EmitLValueForField and then returning nullptr from CodeGenTBAA::getTBAAStructT

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

dberlin added a comment. In https://reviews.llvm.org/D31885#727529, @efriedma wrote: > > Such an effective type change must be more explicit than "i allocated > > typeless memory, and so i can do what i want with it". > > How can you change the effective type of malloc'ed memory in C, if storing

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

dberlin added a comment. In https://reviews.llvm.org/D31885#727519, @dberlin wrote: > In https://reviews.llvm.org/D31885#727499, @hfinkel wrote: > > > In https://reviews.llvm.org/D31885#727371, @efriedma wrote: > > > > > In https://reviews.llvm.org/D31885#727167, @hfinkel wrote: > > > > > > > I'm

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

efriedma added a comment. > Such an effective type change must be more explicit than "i allocated > typeless memory, and so i can do what i want with it". How can you change the effective type of malloc'ed memory in C, if storing a value of a new type doesn't have any effect? memset? A new C

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

dberlin added a comment. In https://reviews.llvm.org/D31885#727499, @hfinkel wrote: > In https://reviews.llvm.org/D31885#727371, @efriedma wrote: > > > In https://reviews.llvm.org/D31885#727167, @hfinkel wrote: > > > > > I'm not sure this is the right way to do this; I suspect we're lumping > >

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

hfinkel added a comment. In https://reviews.llvm.org/D31885#727371, @efriedma wrote: > In https://reviews.llvm.org/D31885#727167, @hfinkel wrote: > > > I'm not sure this is the right way to do this; I suspect we're lumping > > together a bunch of different bugs: > > > > 1. vector types need to h

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

efriedma added a comment. In https://reviews.llvm.org/D31885#727167, @hfinkel wrote: > I'm not sure this is the right way to do this; I suspect we're lumping > together a bunch of different bugs: > > 1. vector types need to have tbaa which makes them alias with their element > types [to be clea

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

dberlin added a comment. In https://reviews.llvm.org/D31885#727370, @dberlin wrote: > In https://reviews.llvm.org/D31885#727352, @hfinkel wrote: > > > > I'm not sure about the solution to #2, because i thought there were very > > > specific points in time at which the effective type could change

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

dberlin added a comment. In https://reviews.llvm.org/D31885#727352, @hfinkel wrote: > > I'm not sure about the solution to #2, because i thought there were very > > specific points in time at which the effective type could change. > > I think this is a key point. I'm not sure that there are spec

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

kparzysz added a comment. I'm not really concerned about the approach here---I can abandon this patch if you have something better. You have two testcases. One is based on an issue that our customer encountered. As long as TBAA doesn't produce false negatives, it's all good. Repository:

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

hfinkel added a comment. > I'm not sure about the solution to #2, because i thought there were very > specific points in time at which the effective type could change. I think this is a key point. I'm not sure that there are specific points that the frontend can deduce: union U { int i;

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

dberlin added a comment. In https://reviews.llvm.org/D31885#727167, @hfinkel wrote: > I'm not sure this is the right way to do this; I suspect we're lumping > together a bunch of different bugs: > > 1. vector types need to have tbaa which makes them alias with their element > types [to be clear

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

hfinkel added a comment. In https://reviews.llvm.org/D31885#727307, @kparzysz wrote: > This is not meant as a fine-grained solution to the TBAA problem, but a > temporary fix for generating wrong information. Just yesterday I helped > diagnose yet another problem related to this, so this issue

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

kparzysz added a comment. This is not meant as a fine-grained solution to the TBAA problem, but a temporary fix for generating wrong information. Just yesterday I helped diagnose yet another problem related to this, so this issue is causing trouble out there. Repository: rL LLVM https://r

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

hfinkel added a comment. I'm not sure this is the right way to do this; I suspect we're lumping together a bunch of different bugs: 1. vector types need to have tbaa which makes them alias with their element types [to be clear, as vector types are an implementation extension, this is our choic

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

kparzysz updated this revision to Diff 95217. kparzysz edited the summary of this revision. kparzysz added a reviewer: hfinkel. kparzysz added a comment. Fixed the testcases (forgot to use FileCheck). Repository: rL LLVM https://reviews.llvm.org/D31885 Files: lib/CodeGen/CGExpr.cpp lib/C

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

kparzysz updated this revision to Diff 94721. kparzysz added a comment. Cleaned the code up, added testcases. Repository: rL LLVM https://reviews.llvm.org/D31885 Files: lib/CodeGen/CGExpr.cpp lib/CodeGen/CodeGenFunction.h test/CodeGen/union-tbaa1.c test/CodeGenCXX/union-tbaa2.cpp In

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

kparzysz created this revision. The TBAA information for union members may be wrong, and in the current form TBAA cannot represent them correctly. This is a follow-up to this thread from llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2017-April/111859.html Repository: rL LLVM https://re

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

kparzysz added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1019 +/// optimizations). +static bool isUnionAccess(const Expr *E) { + switch (E->getStmtClass()) { I'm not sure if this is a complete list of different ways to get a union member. Repository:

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

kparzysz updated this revision to Diff 94699. kparzysz added a comment. Do not stop the check for unions at the first MemberExpr (unless it's a union). Repository: rL LLVM https://reviews.llvm.org/D31885 Files: lib/CodeGen/CGExpr.cpp Index: lib/CodeGen/CGExpr.cpp =

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

wjakob added a comment. Disclaimer: I am not an LLVM developer. Just looking at the patch, I wonder if it could be much less intrusive if `LValue CodeGenFunction::EmitLValue(const Expr *E)` is split into two methods -- a private one, and a public-facing one that applies your `ClearTBAA` method.