-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[TBAA] Fix the case where a subobject gets accessed at a non-zero offset. #101485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-llvm-analysis Author: Ivan Kosarev (kosarev) ChangesFull diff: https://github.com/llvm/llvm-project/pull/101485.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp b/llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp
index 0d7eb7da8d6b6..44d3404e28258 100644
--- a/llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp
@@ -613,12 +613,13 @@ static bool mayBeAccessToSubobjectOf(TBAAStructTagNode BaseTag,
}
if (BaseType.getNode() == SubobjectTag.getBaseType()) {
- bool SameMemberAccess = OffsetInBase == SubobjectTag.getOffset();
+ MayAlias = OffsetInBase == SubobjectTag.getOffset() ||
+ BaseType.getNode() == BaseTag.getAccessType() ||
+ SubobjectTag.getBaseType() == SubobjectTag.getAccessType();
if (GenericTag) {
- *GenericTag = SameMemberAccess ? SubobjectTag.getNode() :
- createAccessTag(CommonType);
+ *GenericTag =
+ MayAlias ? SubobjectTag.getNode() : createAccessTag(CommonType);
}
- MayAlias = SameMemberAccess;
return true;
}
diff --git a/llvm/test/Analysis/TypeBasedAliasAnalysis/aggregates.ll b/llvm/test/Analysis/TypeBasedAliasAnalysis/aggregates.ll
index 4049c78049e03..1c5efa219134e 100644
--- a/llvm/test/Analysis/TypeBasedAliasAnalysis/aggregates.ll
+++ b/llvm/test/Analysis/TypeBasedAliasAnalysis/aggregates.ll
@@ -6,7 +6,7 @@
; Check that TBAA handles access tags with aggregate final access types
; correctly.
-%A = type { i32 } ; struct A { int i; };
+%A = type { i32, i32 } ; struct A { int i, j; };
%B = type { %A } ; struct B { A a; };
%C = type { %B } ; struct C { B b; };
%D = type { i16 } ; struct D { short s; };
@@ -105,13 +105,32 @@ entry:
ret i32 %0
}
+; A vs. A::j => MayAlias.
+; This differs from A vs. A::i case in that the offsets of the final
+; accessed objects in A do not match.
+define i32 @f7(ptr %i, ptr %a) {
+entry:
+; CHECK-LABEL: f7
+; CHECK: MayAlias: store i32 7, {{.*}} <-> store i32 5,
+; OPT-LABEL: f7
+; OPT: store i32 5,
+; OPT: store i32 7,
+; OPT: %[[RET:.*]] = load i32,
+; OPT: ret i32 %[[RET]]
+ store i32 5, ptr %i, align 4, !tbaa !10 ; TAG_A
+ store i32 7, ptr %a, align 4, !tbaa !16 ; TAG_A_j
+ %0 = load i32, ptr %i, align 4, !tbaa !10 ; TAG_A
+ ret i32 %0
+}
+
!0 = !{!"root"}
!1 = !{!0, i64 1, !"char"}
!2 = !{!1, i64 4, !"int"}
!3 = !{!2, !2, i64 0, i64 4} ; TAG_int
-!4 = !{!1, i64 4, !"A", !2, i64 0, i64 4}
+!4 = !{!1, i64 4, !"A", !2, i64 0, i64 4, !2, i64 4, i64 4}
!5 = !{!4, !2, i64 0, i64 4} ; TAG_A_i
+!16 = !{!4, !2, i64 4, i64 4} ; TAG_A_j
!6 = !{!1, i64 4, !"B", !4, i64 0, i64 4}
!7 = !{!6, !4, i64 0, i64 4} ; TAG_B_a
|
|
@vedantparanjape-amd Vedant, can you confirm if this resolves #101164? |
Hello Ivan, sorry for the delay to get back. Let me verify this. It seems similar to my prototype fix ! |
antoniofrighetto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me, please wait for another reviewer's approval.
|
Rebased. Ping. |
| break; | ||
| } | ||
|
|
||
| if (BaseType.getNode() == SubobjectTag.getBaseType()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I can't follow the logic here at all. What is the purpose of the code inside this if statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the type of the current base type matched the type of the subobject type, we see if the accesses within these same-typed objects may alias. So far we were just looking at the offsets, which doesn't work when one of the accesses dereferences the whole object, meaning offset 0, and the other dereferences its field/element at offset other than 0. The change fixes that by catching the case where any of the accesses are whole-object ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see... the old code detects whether both accesses refer to the same object, but we also care about overlapping objects. According to the TBAA rules, objects have to be properly nested. So the only way to get overlap is if one of the accesses is a "whole-object" access. Therefore, this check.
LangRef says "the access type is a scalar type descriptor", which I guess isn't accurate? Please propose a patch to fix the language there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LangRef doesn't cover the new aggregate-enabled TBAA format, which this patch is about. The latest plan documentation-wise I'm aware of is to wait until the new format is mature enough to be enabled by default, and then update the documentation, see https://reviews.llvm.org/D40975#955973. Until then it's all work-in-progress and not something supposed to be exposed to the user.
efriedma-quic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.