[ConstantMerge] Only merge constant w/unnamed_addr

[gulfem]

Summary

Currently, ConstantMergePass merges an unnamed_addr with a
non-unnamed_addr constant as it is explained in LangRef.

"Note that a constant with significant address can be merged with a
unnamed_addr constant, the result being a constant whose address is
significant."

https://llvm.org/docs/LangRef.html#global-variables

This can result in a situation where Clang vioalates C semantics, and
here is a small reproducer to explain the problem:

const char foo_string[] = "foo";
const char* foo_func(void) { return "foo"; }
int is_foo(const char* p) { return p == foo_string; }

int main() {
  printf("is_foo: %d\n", is_foo("foo"));
}

When we compile with -O0, where ConstantMerge is not applied, Clang
and GCC have the same result.

clang -O0 foo.c -o foo
./foo
is_foo: 0

gcc -O0 foo.c -o foo
./foo
is_foo: 0

When we compile -O1 and higher, where ConstantMerge is applied, Clang
and GCC have different results.

clang -O3 foo.c -o foo
./foo
is_foo: 1

gcc -O3 foo.c -o foo
./foo
is_foo: 0

Here's the IR before ConstantMergePass pass:

@.str = private unnamed_addr constant [4 x i8] c"foo\00", align 1
@_ZL10foo_string = internal constant [4 x i8] c"foo\00", align 1
@.str.1 = private unnamed_addr constant [12 x i8] c"is_foo: %d\0A\00",
align 1

; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone
uwtable willreturn
define noundef i8* @_Z8foo_funcv() local_unnamed_addr #0 {
 ret i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0)
}

; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone
uwtable willreturn
define noundef i32 @_Z6is_fooPKc(i8* noundef readnone %0)
local_unnamed_addr #0 {
%2 = icmp eq i8* %0, getelementptr inbounds ([4 x i8], [4 x i8]*
@_ZL10foo_string, i64 0, i64 0)
%3 = zext i1 %2 to i32
ret i32 %3
}

; Function Attrs: mustprogress nofree norecurse nounwind uwtable
define noundef i32 @main() local_unnamed_addr #1 {
%1 = tail call i32 (i8*, ...) @printf(i8* noundef nonnull
dereferenceable(1) getelementptr inbounds ([12 x i8], [12 x i8]*
@.str.1, i64 0, i64 0), i32 noundef zext (i1 icmp eq (i8* getelementptr
inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr
inbounds ([4 x i8], [4 x i8]* @_ZL10foo_string, i64 0, i64 0)) to i32))
ret i32 0
}

MergeConstantPass merges _ZL10foo_string into .str, where it merges
a non-unnamed_addr constant into an unnamed_addr constant.

• IR Dump After ConstantMergePass on [module] ***

@.str = private constant [4 x i8] c"foo\00", align 1
@.str.1 = private unnamed_addr constant [12 x i8] c"is_foo: %d\0A\00",
align 1

; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone
uwtable willreturn
define noundef i8* @_Z8foo_funcv() local_unnamed_addr #0 {
ret i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0)
}

; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone
uwtable willreturn
define noundef i32 @_Z6is_fooPKc(i8* noundef readnone %0)
local_unnamed_addr #0 {
%2 = icmp eq i8* %0, getelementptr inbounds ([4 x i8], [4 x i8]* @.str,
i64 0, i64 0)
%3 = zext i1 %2 to i32
ret i32 %3
}

; Function Attrs: mustprogress nofree norecurse nounwind uwtable
define noundef i32 @main() local_unnamed_addr #1 {
%1 = tail call i32 (i8*, ...) @printf(i8* noundef nonnull
dereferenceable(1) getelementptr inbounds ([12 x i8], [12 x i8]*
@.str.1, i64 0, i64 0), i32 noundef 1)
ret i32 0
}

This transformation violates the following C pointer semantics:

"Two pointers compare equal if and only if both are null pointers,
both are pointers to the same object (including a pointer to an object
and a subobject at its beginning) or function, both are pointers to
one past the last element of the same array object, or one is a pointer
to one past the end of one array object and the other is a pointer to
the start of a different array object that happens to immediately
follow the first array object in the address space."
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2912.pdf

So, this patch changes ConstantMerge pass to only allow merging when
when a constant is marked with unnamed_addr attribute.
I also found an old GitHub issue where a similar issue about invalid
constant merging is explained.
https://github.com/llvm/llvm-project/issues/9299

[gulfem]

Changed the commit message

[eugenis]

I'm far from a C or C++ expert, but FWIW I'm not entirely convinced that this example breaks language semantics.

[efriedma]

The C standard says "It is unspecified whether these arrays are distinct provided their elements have the appropriate values." C++ says "Whether all string-literals are distinct (that is, are stored in nonoverlapping objects) and whether successive evaluations of a string-literal yield the same or a different object is unspecified."

I suppose you could narrowly interpret the standards as only allowing string literals to overlap other string literals, but they don't explicitly say that. And allowing string literals to overlap with other constants hasn't caused any practical issues, as far as I know.

[efriedma]

If we do decide that string literals need to be special, we might want to consider a distinct "unnamed_addr_only" or something like that, so we don't block merging in cases which aren't constrained by the C/C++ standards.

llvm/lib/Transforms/IPO/ConstantMerge.cpp

return !GV->isConstant() || !GV->hasDefinitiveInitializer() ||
          // Don't touch thread-local variables.
          GV->isThreadLocal() ||
          // Don't touch values marked with attribute(used).
          UsedGlobals.count(GV);
 }
 
 enum class CanMerge { No, Yes };
 static CanMerge makeMergeable(GlobalVariable *Old, GlobalVariable *New) {
-  if (!Old->hasGlobalUnnamedAddr() && !New->hasGlobalUnnamedAddr())
+  if (!Old->hasGlobalUnnamedAddr() || !New->hasGlobalUnnamedAddr())
     return CanMerge::No;
   if (hasMetadataOtherThanDebugLoc(Old))
     return CanMerge::No;
   assert(!hasMetadataOtherThanDebugLoc(New));
   if (!Old->hasGlobalUnnamedAddr())
     New->setUnnamedAddr(GlobalValue::UnnamedAddr::None);
   return CanMerge::Yes;
 }

llvm/test/Transforms/ConstantMerge/2011-01-15-EitherOrder.ll

 ; RUN: opt -passes=constmerge -S < %s | FileCheck %s
 ; PR8978
 
 declare i32 @zed(ptr, ptr)
 
 %struct.foobar = type { i32 }
 ; CHECK: bar.d
 @bar.d =  unnamed_addr constant %struct.foobar zeroinitializer, align 4
-; CHECK-NOT: foo.d
+; CHECK: foo.d
 @foo.d = internal constant %struct.foobar zeroinitializer, align 4
 define i32 @main() nounwind ssp {
 entry:
 ; CHECK: bar.d
   %call2 = tail call i32 @zed(ptr @foo.d, ptr @bar.d)
 nounwind
   ret i32 0
 }
 

llvm/test/Transforms/ConstantMerge/merge-both.ll

 ; RUN: opt -S < %s -passes=constmerge | FileCheck %s
-; Test that in one run var3 is merged into var2 and var1 into var4.
-; Test that we merge @var5 and @var6 into one with the higher alignment
+; Test that in one run var2 is merged into var4 and var6 is merged into var8.
+; Test that we merge @var6 and @var8 into one with the higher alignment
 
 declare void @zed(ptr, ptr)
 
 %struct.foobar = type { i32 }
 
 @var1 = internal constant %struct.foobar { i32 2 }
-@var2 = unnamed_addr constant %struct.foobar { i32 2 }
+@var2 = private unnamed_addr constant %struct.foobar { i32 2 }
 @var3 = internal constant %struct.foobar { i32 2 }
-@var4 = unnamed_addr constant %struct.foobar { i32 2 }
+@var4 = private unnamed_addr constant %struct.foobar { i32 2 }
 
 ; CHECK:      %struct.foobar = type { i32 }
 ; CHECK-NOT: @
-; CHECK: @var2 = constant %struct.foobar { i32 2 }
-; CHECK-NEXT: @var4 = constant %struct.foobar { i32 2 }
+; CHECK: @var1 = internal constant %struct.foobar { i32 2 }
+; CHECK-NEXT: @var3 = internal constant %struct.foobar { i32 2 }
+; CHECK-NEXT: @var4 = private unnamed_addr constant %struct.foobar { i32 2 }
 
 declare void @helper(ptr)
 @var5 = internal constant [16 x i8] c"foo1bar2foo3bar\00", align 16
 @var6 = private unnamed_addr constant [16 x i8] c"foo1bar2foo3bar\00", align 1
 @var7 = internal constant [16 x i8] c"foo1bar2foo3bar\00"
 @var8 = private unnamed_addr constant [16 x i8] c"foo1bar2foo3bar\00"
 
+; CHECK: @var5 = internal constant [16 x i8] c"foo1bar2foo3bar\00", align 16
 ; CHECK-NEXT: @var7 = internal constant [16 x i8] c"foo1bar2foo3bar\00"
-; CHECK-NEXT: @var8 = private constant [16 x i8] c"foo1bar2foo3bar\00", align 16
+; CHECK-NEXT: @var8 = private unnamed_addr constant [16 x i8] c"foo1bar2foo3bar\00", align 1
 
 @var4a = alias %struct.foobar, ptr @var4
 @llvm.used = appending global [1 x ptr] [ptr @var4a], section "llvm.metadata"
 
 define i32 @main() {
 entry:
   call void @zed(ptr @var1, ptr @var2)
   call void @zed(ptr @var3, ptr @var4)
   call void @helper(ptr @var5)
   call void @helper(ptr @var6)
   call void @helper(ptr @var7)
   call void @helper(ptr @var8)
   ret i32 0
 }
-

llvm/test/Transforms/ConstantMerge/merge-dbg.ll

 ; RUN: opt < %s -passes=constmerge -S | FileCheck %s
 
-; CHECK: = constant i32 1, !dbg [[A:![0-9]+]], !dbg [[B:![0-9]+]]
-@a = internal constant i32 1, !dbg !0
-@b = unnamed_addr constant i32 1, !dbg !9
+; CHECK: = private unnamed_addr constant i32 1, !dbg [[A:![0-9]+]], !dbg [[B:![0-9]+]]
+@a = private unnamed_addr constant i32 1, !dbg !0
+@b = private unnamed_addr constant i32 1, !dbg !9
 
 define void @test1(ptr %P1, ptr %P2) {
   store ptr @a, ptr %P1
   store ptr @b, ptr %P2
   ret void
 }
 
 !llvm.dbg.cu = !{!2}

llvm/test/Transforms/ConstantMerge/unnamed-addr.ll

 ; RUN: opt -passes=constmerge -S < %s | FileCheck %s
-; Test which corresponding x and y are merged and that unnamed_addr
-; is correctly set.
+; Test which corresponding x and y are merged when they both are marked with
+; unnamed_addr attribute.
 
 declare void @zed(ptr, ptr)
 
 %struct.foobar = type { i32 }
 
 @test1.x = internal constant %struct.foobar { i32 1 }
 @test1.y = constant %struct.foobar { i32 1 }
 
 @test2.x = internal constant %struct.foobar { i32 2 }
 @test2.y = unnamed_addr constant %struct.foobar { i32 2 }
 
 @test3.x = internal unnamed_addr constant %struct.foobar { i32 3 }
 @test3.y = constant %struct.foobar { i32 3 }
 
 @test4.x = internal unnamed_addr constant %struct.foobar { i32 4 }
 @test4.y = unnamed_addr constant %struct.foobar { i32 4 }
 
 
 ; CHECK:      %struct.foobar = type { i32 }
 ; CHECK-NOT: @
 ; CHECK: @test1.x = internal constant %struct.foobar { i32 1 }
 ; CHECK-NEXT: @test1.y = constant %struct.foobar { i32 1 }
-; CHECK-NEXT: @test2.y = constant %struct.foobar { i32 2 }
+; CHECK-NEXT: @test2.x = internal constant %struct.foobar { i32 2 }
+; CHECK-NEXT: @test2.y = unnamed_addr constant %struct.foobar { i32 2 }
+; CHECK-NEXT: @test3.x = internal unnamed_addr constant %struct.foobar { i32 3 }
 ; CHECK-NEXT: @test3.y = constant %struct.foobar { i32 3 }
 ; CHECK-NEXT: @test4.y = unnamed_addr constant %struct.foobar { i32 4 }
 ; CHECK-NOT: @
 ; CHECK: declare void @zed(ptr, ptr)
 
 define i32 @main() {
 entry:
   call void @zed(ptr @test1.x, ptr @test1.y)
   call void @zed(ptr @test2.x, ptr @test2.y)
   call void @zed(ptr @test3.x, ptr @test3.y)
   call void @zed(ptr @test4.x, ptr @test4.y)
   ret i32 0
 }