GlobalOpt should maintain externally_initialized when splitting aggregates

[olista01]

Summary

When GlobalOpt splits an internal, global variable with an aggregate type, it should propagate the externally_initialized flag to the newly created globals.

This makes the pass safe for our downstream use of this flag, while still allowing some useful optimisations (such as removing dead parts of the split aggregate) to be performed. However, I'm not familiar enough with the other uses of this flag (Objective-C selectors and the CUDA device qualifier) to know if splitting an internal, externally_initialized global is valid for them. I'm guessing that it is safe for internal globals, but could anyone confirm this?

I've got a related patch up for review at D13343, the test in this patch depends on that change.

[olista01]

Ping

[olista01]

Ping

[olista01]

Ping

[olista01]

Ping? (and adding some reviewers that may be able to answer the question about CUDA and Objective-C)

[olista01]

You LGTM’d a related patch (D13343), I had assumed that you did not review this one because of the CUDA and Objective-C questions (in the phabricator summary).

Oliver

From: James Molloy [mailto:[e-mail]]
Sent: 09 November 2015 16:27
To: [e-mail]; Oliver Stannard; [e-mail]; [e-mail]
Cc: [e-mail]
Subject: Re: [PATCH] D13382: GlobalOpt should maintain externally_initialized when splitting aggregates

Hi Oliver,

I did LGTM this on the 19th October. Did you want more reviews?

James

[olista01]

Ah, sorry, my mistake. I see it in the mail thread, but not in Phab.

I don’t think there’s any need for further review.

From: James Molloy [mailto:[e-mail]]
Sent: 09 November 2015 16:36
To: Oliver Stannard; [e-mail]; [e-mail]; [e-mail]
Cc: [e-mail]
Subject: Re: [PATCH] D13382: GlobalOpt should maintain externally_initialized when splitting aggregates

I did LGTM this one specifically - I see it in the same mail thread.

I felt the behaviour was obvious and shouldn't depend on ObjC selector support - it looks correct according to LangRef.

However if you want someone else to chip in I'm not going to stand in your way :)

James

llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp

if (StructType *STy = dyn_cast<StructType>(Ty)) {
     for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
       Constant *In = Init->getAggregateElement(i);
       assert(In && "Couldn't get element of initializer?");
       GlobalVariable *NGV = new GlobalVariable(STy->getElementType(i), false,
                                                GlobalVariable::InternalLinkage,
                                                In, GV->getName()+"."+Twine(i),
                                                GV->getThreadLocalMode(),
                                               GV->getType()->getAddressSpace());
+      NGV->setExternallyInitialized(GV->isExternallyInitialized());
       Globals.insert(GV->getIterator(), NGV);
       NewGlobals.push_back(NGV);
 
       // Calculate the known alignment of the field.  If the original aggregate
       // had 256 byte alignment for example, something might depend on that:
       // propagate info to each field.
       uint64_t FieldOffset = Layout.getElementOffset(i);
       unsigned NewAlign = (unsigned)MinAlign(StartAlignment, FieldOffset);
for (unsigned i = 0, e = NumElements; i != e; ++i) {
       Constant *In = Init->getAggregateElement(i);
       assert(In && "Couldn't get element of initializer?");
 
       GlobalVariable *NGV = new GlobalVariable(STy->getElementType(), false,
                                                GlobalVariable::InternalLinkage,
                                                In, GV->getName()+"."+Twine(i),
                                                GV->getThreadLocalMode(),
                                               GV->getType()->getAddressSpace());
+      NGV->setExternallyInitialized(GV->isExternallyInitialized());
       Globals.insert(GV->getIterator(), NGV);
       NewGlobals.push_back(NGV);
 
       // Calculate the known alignment of the field.  If the original aggregate
       // had 256 byte alignment for example, something might depend on that:
       // propagate info to each field.
       unsigned NewAlign = (unsigned)MinAlign(StartAlignment, EltSize*i);
       if (NewAlign > EltAlign)

llvm/trunk/test/Transforms/GlobalOpt/externally-initialized-aggregate.ll

+; RUN: opt < %s -S -globalopt | FileCheck %s
+
+; This global is externally_initialized, so if we split it into scalars we
+; should keep that flag set on all of the new globals. This will prevent the
+; store to @a[0] from being constant propagated to the load in @foo, but will not
+; prevent @a[1] from being removed since it is dead.
+; CHECK: @a.0 = internal unnamed_addr externally_initialized global i32 undef
+; CHECK-NOT @a.1
+@a = internal externally_initialized global [2 x i32] undef, align 4
+; This is the same, but a struct rather than an array.
+; CHECK: @b.0 = internal unnamed_addr externally_initialized global i32 undef
+; CHECK-NOT @b.1
+@b = internal externally_initialized global {i32, i32} undef, align 4
+
+define i32 @foo() {
+; CHECK-LABEL: define i32 @foo
+entry:
+; This load uses the split global, but cannot be constant-propagated away.
+; CHECK: %0 = load i32, i32* @a.0
+  %0 = load i32, i32* getelementptr inbounds ([2 x i32], [2 x i32]* @a, i32 0, i32 0), align 4
+  ret i32 %0
+}
+
+define i32 @bar() {
+; CHECK-LABEL: define i32 @bar
+entry:
+; This load uses the split global, but cannot be constant-propagated away.
+; CHECK: %0 = load i32, i32* @b.0
+  %0 = load i32, i32* getelementptr inbounds ({i32, i32}, {i32, i32}* @b, i32 0, i32 0), align 4
+  ret i32 %0
+}
+
+define void @init() {
+; CHECK-LABEL: define void @init
+entry:
+; This store uses the split global, but cannot be constant-propagated away.
+; CHECK: store i32 1, i32* @a.0
+  store i32 1, i32* getelementptr inbounds ([2 x i32], [2 x i32]* @a, i32 0, i32 0), align 4
+; This store can be removed, because the second element of @a is never read.
+; CHECK-NOT: store i32 2, i32* @a.1
+  store i32 2, i32* getelementptr inbounds ([2 x i32], [2 x i32]* @a, i32 0, i32 1), align 4
+
+; This store uses the split global, but cannot be constant-propagated away.
+; CHECK: store i32 3, i32* @b.0
+  store i32 3, i32* getelementptr inbounds ({i32, i32}, {i32, i32}* @b, i32 0, i32 0), align 4
+; This store can be removed, because the second element of @b is never read.
+; CHECK-NOT: store i32 4, i32* @b.1
+  store i32 4, i32* getelementptr inbounds ({i32, i32}, {i32, i32}* @b, i32 0, i32 1), align 4
+  ret void
+}