Page MenuHomePhabricator

[CodeGen] Pessimize aliasing for union members (and may-alias) objects
ClosedPublic

Authored by kparzysz on May 18 2017, 11:22 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz updated this revision to Diff 99467.May 18 2017, 11:28 AM
kparzysz edited subscribers, added: cfe-commits; removed: llvm-commits.

The diff is the same as before, the change is in the subscribers: from llvm-commits to cfe-commits.

kparzysz updated this revision to Diff 99918.May 23 2017, 8:29 AM
kparzysz edited the summary of this revision. (Show Details)

Previously, the variable mayAlias in EmitLValueForField could have been false even if the base LValue had may-alias flag set. Make sure that mayAlias reflects the base aliasing.

This fixed the C++ testcase and it's now included in the patch.

hfinkel added inline comments.May 23 2017, 9:25 AM
test/CodeGen/union-tbaa1.c
44 ↗(On Diff #99918)

Please check the path offset?

test/CodeGenCXX/union-tbaa2.cpp
45 ↗(On Diff #99918)

Please check the path offset.

kparzysz added inline comments.May 23 2017, 9:28 AM
test/CodeGen/union-tbaa1.c
44 ↗(On Diff #99918)

What needs to be checked exactly? The offset will be 0, do you want to check for that?

hfinkel added inline comments.May 23 2017, 9:32 AM
test/CodeGen/union-tbaa1.c
44 ↗(On Diff #99918)

Yes, because if it's not zero, this won't have the desired effect.

kparzysz updated this revision to Diff 99939.May 23 2017, 9:51 AM

Checking for offset 0 in the TBAA info.

kparzysz marked 2 inline comments as done.May 23 2017, 9:51 AM
rjmccall added inline comments.May 23 2017, 1:21 PM
lib/CodeGen/CGExpr.cpp
1436 ↗(On Diff #99939)

Hmm. Should we be constructing a struct-path TBAA at all if the base may alias, as opposed to just using 'char'?

kparzysz added inline comments.May 23 2017, 2:33 PM
lib/CodeGen/CGExpr.cpp
1436 ↗(On Diff #99939)

I did that and got a verifier error: "Old-style TBAA is no longer allowed, use struct-path TBAA instead. fatal error: error in backend: Broken function found, compilation aborted!"

rjmccall added inline comments.May 23 2017, 3:43 PM
lib/CodeGen/CGExpr.cpp
1436 ↗(On Diff #99939)

Oh, I haven't been paying enough attention to LLVM's TBAA changes. It looks like they've canonicalized on always using the struct-access-style TBAA metadata, which is fine; the point is that we should be using whatever tag we would use for dereferencing a simple char*, which I believe just means passing true for ConvertTypeToTag to DecorateInstructionWithTBAA .

kparzysz added inline comments.May 24 2017, 5:49 AM
lib/CodeGen/CGExpr.cpp
1436 ↗(On Diff #99939)

With that change I'm getting another error:

Access type node must be a valid scalar type
  store <4 x double> %call2, <4 x double>* %b, align 32, !tbaa !8
!8 = !{!9, !9, i64 0}
!9 = !{!4, !4, i64 0}
fatal error: error in backend: Broken function found, compilation aborted!
rjmccall added inline comments.May 24 2017, 11:22 AM
lib/CodeGen/CGExpr.cpp
1436 ↗(On Diff #99939)

Sorry, I see now that what I said was unclear. If you do this, you need to pass the result of getTBAAInfo(CharTy) down as the argument to DecorateInstructionWithTBAA instead of building of a TBAA struct tag.

kparzysz updated this revision to Diff 100142.May 24 2017, 11:40 AM

Pass char TBAA directly to DecorateInstructionWithTBAA and pass true for ConvertTypeToTag in such cases.

kparzysz marked an inline comment as done.May 24 2017, 11:41 AM
rjmccall accepted this revision.May 24 2017, 3:58 PM

Thanks, that looks great.

This revision is now accepted and ready to land.May 24 2017, 3:58 PM
This revision was automatically updated to reflect the committed changes.