[llvm] [AST] Fix size merging for MustAlias sets (PR #73820)

[Nikita Popov via llvm-commits]

https://github.com/nikic created https://github.com/llvm/llvm-project/pull/73820

AST checks aliasing with MustAlias sets by only checking the representative pointer (getSomePointer). This is only correct if the Size and AATags information of that pointer also includes the Size/AATags of all other pointers in the set.

When we add a new pointer to the AliasSet, we do perform this update (see the code in AliasSet::addPointer). However, if a pointer already in the MustAlias set is used with a new size, we currently do not update the representative pointer, resulting in miscompilations. Fix this by adding the missing update.

This is targeted fix using the current representation. There are a couple of alternatives:
 * For MustAlias sets, don't store per-pointer Size/AATags at all. This would make it clear that there is only one set of common Size/AATags for all pointers.
 * Check against all pointers in the set even for MustAlias. This is what https://github.com/llvm/llvm-project/pull/65731 proposes to do as part of a larger change to AST representation.

Fixes https://github.com/llvm/llvm-project/issues/64897.

>From d088d17ff7ac1204272f9479f0fe37ceecca0b1f Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Wed, 29 Nov 2023 17:56:30 +0100
Subject: [PATCH] [AST] Fix size merging for MustAlias sets

AST checks aliasing with MustAlias sets by only checking the
representative pointer (getSomePointer). This is only correct
if the Size and AATags information of that pointer also includes
the Size/AATags of all other pointers in the set.

When we add a new pointer to the AliasSet, we do perform this
update (see the code in AliasSet::addPointer). However, if a
pointer already in the MustAlias set is used with a new size,
we currently do not update the representative pointer, resulting
in miscompilations. Fix this by adding the missing update.

This is targeted fix using the current representation. There are
a couple of alternatives:
 * For MustAlias sets, don't store per-pointer Size/AATags at all.
   This would make it clear that there is only one set of common
   Size/AATags for all pointers.
 * Check against all pointers in the set even for MustAlias. This
   is what https://github.com/llvm/llvm-project/pull/65731
   proposes to do as part of a larger change to AST representation.

Fixes https://github.com/llvm/llvm-project/issues/64897.
---
 llvm/lib/Analysis/AliasSetTracker.cpp | 10 +++++++++-
 llvm/test/Transforms/LICM/pr64897.ll  |  8 +++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/llvm/lib/Analysis/AliasSetTracker.cpp b/llvm/lib/Analysis/AliasSetTracker.cpp
index 91b889116dfa2d3..debdd328ce53fdc 100644
--- a/llvm/lib/Analysis/AliasSetTracker.cpp
+++ b/llvm/lib/Analysis/AliasSetTracker.cpp
@@ -348,8 +348,16 @@ AliasSet &AliasSetTracker::getAliasSetFor(const MemoryLocation &MemLoc) {
     // due to a quirk of alias analysis behavior. Since alias(undef, undef)
     // is NoAlias, mergeAliasSetsForPointer(undef, ...) will not find the
     // the right set for undef, even if it exists.
-    if (Entry.updateSizeAndAAInfo(Size, AAInfo))
+    if (Entry.updateSizeAndAAInfo(Size, AAInfo)) {
       mergeAliasSetsForPointer(Pointer, Size, AAInfo, MustAliasAll);
+
+      // For MustAlias sets, also update Size/AAInfo of the representative
+      // pointer.
+      AliasSet &AS = *Entry.getAliasSet(*this);
+      if (AS.isMustAlias())
+        if (AliasSet::PointerRec *P = AS.getSomePointer())
+          P->updateSizeAndAAInfo(Size, AAInfo);
+    }
     // Return the set!
     return *Entry.getAliasSet(*this)->getForwardedTarget(*this);
   }
diff --git a/llvm/test/Transforms/LICM/pr64897.ll b/llvm/test/Transforms/LICM/pr64897.ll
index 1ce78f15d797359..12b11eb6912b0cb 100644
--- a/llvm/test/Transforms/LICM/pr64897.ll
+++ b/llvm/test/Transforms/LICM/pr64897.ll
@@ -1,7 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
 ; RUN: opt -S -passes=licm < %s | FileCheck %s
 
-; FIXME: This is a miscompile.
 define void @test(i1 %c, i8 %x) {
 ; CHECK-LABEL: define void @test(
 ; CHECK-SAME: i1 [[C:%.*]], i8 [[X:%.*]]) {
@@ -10,17 +9,16 @@ define void @test(i1 %c, i8 %x) {
 ; CHECK-NEXT:    [[P:%.*]] = getelementptr inbounds i8, ptr [[A]], i64 8
 ; CHECK-NEXT:    [[P_COPY:%.*]] = getelementptr inbounds i8, ptr [[A]], i64 8
 ; CHECK-NEXT:    [[P2:%.*]] = getelementptr inbounds i8, ptr [[A]], i64 12
-; CHECK-NEXT:    [[P2_PROMOTED:%.*]] = load i8, ptr [[P2]], align 1
 ; CHECK-NEXT:    br label [[LOOP:%.*]]
 ; CHECK:       loop:
-; CHECK-NEXT:    [[TMP0:%.*]] = phi i8 [ 0, [[LOOP]] ], [ [[P2_PROMOTED]], [[START:%.*]] ]
 ; CHECK-NEXT:    store i32 286331153, ptr [[P]], align 4
 ; CHECK-NEXT:    store i32 34, ptr [[P_COPY]], align 4
 ; CHECK-NEXT:    store i64 3689348814741910323, ptr [[P_COPY]], align 4
-; CHECK-NEXT:    call void @use(i8 [[TMP0]])
+; CHECK-NEXT:    [[VAL:%.*]] = load i8, ptr [[P2]], align 1
+; CHECK-NEXT:    call void @use(i8 [[VAL]])
+; CHECK-NEXT:    store i8 0, ptr [[P2]], align 1
 ; CHECK-NEXT:    br i1 [[C]], label [[LOOP]], label [[EXIT:%.*]]
 ; CHECK:       exit:
-; CHECK-NEXT:    store i8 0, ptr [[P2]], align 1
 ; CHECK-NEXT:    ret void
 ;
 start: