Make sure that any new and optimized objects created during GlobalOPT copy all the attributes from the base object.

[slarin]

Summary

Make sure that any new and optimized objects created during GlobalOPT copy all the attributes from the base object.

A good example of improper behavior in the current implementation is section information associated with the GlobalObject. If a section was set for it, and GlobalOpt is creating/modifying a new object based on this one (often copying the original name), without this change new object will be placed in a default section, resulting in inappropriate properties of the new variable.
The argument here is that if customer specified a section for a variable, any changes to it that compiler does should not cause it to change that section allocation.
Moreover, any other properties worth representation in copyAttributesFrom() should also be propagated.

[mehdi_amini]

I thought we were conservatively avoiding to mess around with variable that have a section (address space?) attached. How do we know that we can split an array or a structure that has a section attached? Couldn't is be mapped to some hardware?

lib/Transforms/IPO/GlobalOpt.cpp

503

Nit: moving it one line above seems nicer to me.

538

same

[slarin]

Not really. See the test case - even if we mix GV with and without sections they will all be processed.

...but I think my point is slightly different - I cannot afford _not_ to do GlobalOpt - it is rather profitable for my use case. We have been using proposed fix in our codebase for a very long time now and I am pretty confident in this use scenario. The big question - is there anyone out there who assumes otherwise...? I see no .lit tests for the contrary, so I assume not.

[mehdi_amini]

I know that global opt is not conservative enough, but I thought the conservative fix was to update any pass that "optimize" with variable with section and prevent it to do so, i.e. I had in mind what Chris wrote here: http://lists.llvm.org/pipermail/llvm-dev/2015-September/090587.html
(Note: I'm not opposed to this, I'm just not knowledgeable enough about what can go wrong)

[slarin]

Yes, We had this discussion with Chris (see more context here)

http://lists.llvm.org/pipermail/llvm-dev/2015-September/090923.html

...but I read his final comment as a comment, not as a hard suggestion... Partially for that reason I propose something that looks for me as a simpler solution, but you are right, I really want to make sure enough opinions are expressed.

In your current universe - will this change cause any issues? Will it matter at all? Would it help anything?

---

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[tobiasvk]

The most common uses cases for explicit sections I'm aware of here are situations where the user wants to place part of the code into special memory (e.g. tightly-coupled memory). You're free to optimize within that special section - in fact, that's even desired because the code tends to be very performance-critical (but space-constrained) - but introducing additional control or data edges to other sections is a no-go (which is what we're currently doing in GlobalOpt). The proposed patch would solve the problem nicely.

[• rafael]

This is probably OK with the nits addressed since it doesn't introduce new optimizations, just makes existing ones a bit less objectionable.

test/Transforms/GlobalOpt/GSROA-section.ll

6

You are checking the section, not the address space.

test/Transforms/GlobalOpt/MallocSROA-section.ll

10

You can probably clean up this test. You don't need the numf2s global, init_net can just take an argument to store, you don't need the nounwind marker, etc.

[slarin]

Hi Rafael,

Not really. See the test case - even if we mix GV with and without sections they will all be processed.

...but I think my point is slightly different - I cannot afford _not_ to do GlobalOpt - it is rather profitable for my use case. We have been using proposed fix in our codebase for a very long time now and I am pretty confident in this use scenario. The big question - is there anyone out there who assumes otherwise...? I see no .lit tests for the contrary, so I assume not.

Can you give an overview of what you are using the section for?

Cheers,
Rafael

We have to perform LTO on embedded project with hard section assignment. Part of this assignment is driven by pragmas in customer code. GlobalOPT is rather profitable for the kind of code we optimize.

Nevertheless, this change in its own right is independent from our final goal. The point of this patch is that LLVM IR semantics are not preserved by GlobalOPT for no good reason even _outside_ of LTO context... In all of the introduced tests I take legal LLVM IR and use a basic compiler transformation on it with the net result of loss of some IR attributes... ...and the fix is trivial!

If you are conceptually OK with this fix, I will update MallocSROA-section.ll and do the cosmetic changes Mehdi requested in the next patch.

test/Transforms/GlobalOpt/GSROA-section.ll

6

Yes, this is the intent. Without this patch "struct" uses its original section assignment.

[• rafael]

If you are conceptually OK with this fix, I will update MallocSROA-section.ll and do the cosmetic changes Mehdi requested in the next patch.

Sure.

Comment at: test/Transforms/GlobalOpt/GSROA-section.ll:6
@@ +5,3 @@
+; RUN: opt < %s -globalopt -S | FileCheck %s
+; Check that the new global values still have their address space

+; CHECK: @struct

rafael wrote:

You are checking the section, not the address space.

Yes, this is the intent. Without this patch "struct" uses its original section assignment.

But then the comment is out of sync.

[slarin]

Implemented suggested changes.

Simplified, but could not fully change MallocSROA-section.ll since for global OPT to trigger properly and not to optimize out the whole test, both local and global objects seem to be needed.

[• rafael]

This is fine by me, but give Mehdi a chance to comment too.

test/Transforms/GlobalOpt/GSROA-section.ll

12

Please drop the addresspace or check it.

[slarin]

Cleared addrspace in tests.

[mehdi_amini]

Sorry I missed Rafael comment.
LGTM.

lib/Transforms/IPO/GlobalOpt.cpp

for (unsigned i = 0, e = STy->getNumElements(); i != e; ++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.push_back(NGV);
+      NGV->copyAttributesFrom(GV);
       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);
       if (NewAlign > DL.getABITypeAlignment(STy->getElementType(i)))
for (unsigned i = 0, e = NumElements; i != e; ++i) {
 
       GlobalVariable *NGV = new GlobalVariable(STy->getElementType(), false,
                                                GlobalVariable::InternalLinkage,
                                                In, GV->getName()+"."+Twine(i),
                                                GV->getThreadLocalMode(),
                                               GV->getType()->getAddressSpace());
       NGV->setExternallyInitialized(GV->isExternallyInitialized());
       Globals.push_back(NGV);
+      NGV->copyAttributesFrom(GV);
       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)
         NGV->setAlignment(NewAlign);
static GlobalVariable *PerformHeapAllocSRoA(GlobalVariable *GV, CallInst *CI,
   for (unsigned FieldNo = 0, e = STy->getNumElements(); FieldNo != e;++FieldNo){
     Type *FieldTy = STy->getElementType(FieldNo);
     PointerType *PFieldTy = PointerType::get(FieldTy, AS);
 
     GlobalVariable *NGV = new GlobalVariable(
         *GV->getParent(), PFieldTy, false, GlobalValue::InternalLinkage,
         Constant::getNullValue(PFieldTy), GV->getName() + ".f" + Twine(FieldNo),
         nullptr, GV->getThreadLocalMode());
+    NGV->copyAttributesFrom(GV);
     FieldGlobals.push_back(NGV);
 
     unsigned TypeSize = DL.getTypeAllocSize(FieldTy);
     if (StructType *ST = dyn_cast<StructType>(FieldTy))
       TypeSize = DL.getStructLayout(ST)->getSizeInBytes();
     Type *IntPtrTy = DL.getIntPtrType(CI->getType());
     Value *NMI = CallInst::CreateMalloc(CI, IntPtrTy, FieldTy,
                                         ConstantInt::get(IntPtrTy, TypeSize),
static bool TryToShrinkGlobalToBoolean(GlobalVariable *GV, Constant *OtherVal) {
   // Create the new global, initializing it to false.
   GlobalVariable *NewGV = new GlobalVariable(Type::getInt1Ty(GV->getContext()),
                                              false,
                                              GlobalValue::InternalLinkage,
                                         ConstantInt::getFalse(GV->getContext()),
                                              GV->getName()+".b",
                                              GV->getThreadLocalMode(),
                                              GV->getType()->getAddressSpace());
+  NewGV->copyAttributesFrom(GV);
   GV->getParent()->getGlobalList().insert(GV->getIterator(), NewGV);
 
   Constant *InitVal = GV->getInitializer();
   assert(InitVal->getType() != Type::getInt1Ty(GV->getContext()) &&
          "No reason to shrink to bool!");
 
   // If initialized to zero and storing one into the global, we can use a cast
   // instead of a select to synthesize the desired value.

test/Transforms/GlobalOpt/GSROA-section.ll

+; This test lets globalopt split the global struct and array into different
+; values. This used to crash, because globalopt forgot to put the new var in the
+; same address space as the old one.
+
+; RUN: opt < %s -globalopt -S | FileCheck %s
+; Check that the new global values still have their address space
+; CHECK: @struct
+; CHECK: section ".foo"
+; CHECK: @array
+; CHECK-NOT: section ".foo"
+
+@struct = internal addrspace(1) global { i32, i32 } zeroinitializer, section ".foo"
+@array = internal addrspace(1) global [ 2 x i32 ] zeroinitializer
+
+define i32 @foo() {
+  %A = load i32, i32 addrspace(1) * getelementptr ({ i32, i32 }, { i32, i32 } addrspace(1) * @struct, i32 0, i32 0)
+  %B = load i32, i32 addrspace(1) * getelementptr ([ 2 x i32 ], [ 2 x i32 ] addrspace(1) * @array, i32 0, i32 0)
+  ; Use the loaded values, so they won't get removed completely
+  %R = add i32 %A, %B
+  ret i32 %R
+}
+
+; We put stores in a different function, so that the global variables won't get
+; optimized away completely.
+define void @bar(i32 %R) {
+  store i32 %R, i32 addrspace(1) * getelementptr ([ 2 x i32 ], [ 2 x i32 ] addrspace(1) * @array, i32 0, i32 0)
+  store i32 %R, i32 addrspace(1) * getelementptr ({ i32, i32 }, { i32, i32 } addrspace(1) * @struct, i32 0, i32 0)
+  ret void
+}
+
+

test/Transforms/GlobalOpt/MallocSROA-section.ll

+; RUN: opt -globalopt -S < %s | FileCheck %s
+; CHECK: @Y.f0
+; CHECK: section ".foo"
+; CHECK: @Y.f1
+; CHECK: section ".foo"
+
+%struct.xyz = type { double, i32 }
+
+@Y = internal global %struct.xyz* null ,section ".foo"            ; <%struct.xyz**> [#uses=2]
+@numf2s = external global i32                     ; <i32*> [#uses=1]
+
+define fastcc void @init_net() nounwind {
+entry:
+  %0 = load i32, i32* @numf2s, align 4                 ; <i32> [#uses=1]
+  %mallocsize2 = shl i32 %0, 4                    ; <i32> [#uses=1]
+  %malloccall3 = tail call i8* @malloc(i32 %mallocsize2) nounwind ; <i8*> [#uses=1]
+  %1 = bitcast i8* %malloccall3 to %struct.xyz*   ; <%struct.xyz*> [#uses=1]
+  store %struct.xyz* %1, %struct.xyz** @Y, align 8
+  ret void
+}
+
+define fastcc void @load_train(i8* %trainfile, i32 %mode, i32 %objects) nounwind {
+entry:
+  %0 = load %struct.xyz*, %struct.xyz** @Y, align 8             ; <%struct.xyz*> [#uses=0]
+  ret void
+}
+
+declare noalias i8* @malloc(i32)

test/Transforms/GlobalOpt/SROA-section.ll

+; Verify that section assignment is copied during SROA
+; RUN: opt < %s -globalopt -S | FileCheck %s
+; CHECK: @G.0
+; CHECK: section ".foo"
+; CHECK: @G.1
+; CHECK: section ".foo"
+; CHECK: @G.2
+; CHECK: section ".foo"
+
+%T = type { double, double, double }
+@G = internal global %T zeroinitializer, align 16, section ".foo"
+
+define void @test() {
+  store double 1.0, double* getelementptr (%T, %T* @G, i32 0, i32 0), align 16
+  store double 2.0, double* getelementptr (%T, %T* @G, i32 0, i32 1), align 8
+  store double 3.0, double* getelementptr (%T, %T* @G, i32 0, i32 2), align 16
+  ret void
+}
+
+define double @test2() {
+  %V1 = load double, double* getelementptr (%T, %T* @G, i32 0, i32 0), align 16
+  %V2 = load double, double* getelementptr (%T, %T* @G, i32 0, i32 1), align 8
+  %V3 = load double, double* getelementptr (%T, %T* @G, i32 0, i32 2), align 16
+  %R = fadd double %V1, %V2
+  %R2 = fadd double %R, %V3
+  ret double %R2
+}