GlobalOPT and sections

Sergei_Larin · September 18, 2015, 5:45pm

Hello everyone,

This is a rather ancient code with Chris's name all over it, so I naturally feel humbled

I see a conceptual issue in lib/Transforms/IPO/GlobalOpt.cpp with several optimizations that create a copy of GlobalVariable without copying attributes from the original one.

Consider this one:

http://llvm.org/doxygen/GlobalOpt_8cpp_source.html

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());
  GV->getParent()->getGlobalList().insert(GV, NewGV);
...
  // Retain the name of the old global variable. People who are debugging their
  // programs may expect these variables to be named the same.
  NewGV->takeName(GV);
  GV->eraseFromParent();

What I do not see - the section information from the original GV is never copied to the NewGV, so this test would be failing for me:

; RUN: opt < %s -S -globalopt -instcombine | FileCheck %s
;; check that global opt turns integers into bools and keeps section info.

@G = internal addrspace(1) global i32 0, section ".foo"
; CHECK: @G
; CHECK: addrspace(1)
; CHECK: global i1 false
; CHECK: section ".foo"

define void @set1() {
store i32 0, i32 addrspace(1)* @G
; CHECK: store i1 false
ret void
}

define void @set2() {
store i32 1, i32 addrspace(1)* @G
; CHECK: store i1 true
ret void
}

define i1 @get() {
; CHECK-LABEL: @get(
  %A = load i32, i32 addrspace(1) * @G
  %C = icmp slt i32 %A, 2
  ret i1 %C
; CHECK: ret i1 true
}

Now the proverbial question - is this a bug or a feature? ...and if it was meant to be done that way then why...

Beware, that this is the way it is done throughout the file, not just in this specific instance.

Thanks a lot.

Sergei Larin

Chris_Lattner · September 20, 2015, 10:28pm

Historically speaking, this optimization probably predated the ability to put a section on a global.

That said, I think that this is a feature: The sorts of optimizations that globalopt does are extremely transformative to the underlying global variable, and whatever reason the global was assigned to a section in the first place is probably not going to work with the transformed value.

If you’re having a problem with this, it is probably best to just make globalopt more conservative: have it refuse to transform a global that has any explicitly assigned section.

-Chris

Sergei_Larin · September 21, 2015, 6:59pm

Chris,

Thanks for the clarification... at least no bug report is due... and I am glad that I've asked.

In my case these transformations are rather useful and forcing them to copy original global variable section is making them compatible with our (rather important) use case, so I guess I will have to fix it locally.
Nevertheless if someone else would have a similar issue - I would be happy to patch it.

Sergei

Chris_Lattner · September 22, 2015, 3:36am

Chris,

Thanks for the clarification... at least no bug report is due... and I am glad that I've asked.

In my case these transformations are rather useful and forcing them to copy original global variable section is making them compatible with our (rather important) use case, so I guess I will have to fix it locally.
Nevertheless if someone else would have a similar issue - I would be happy to patch it.

Ah, so you’re saying that you *want* the transformation, but the global has an explicit section?

Two questions:
1) Why does the global have a section? Is this something that should be better modeled in IR somehow?
2) Perhaps this transformation can be better modeled as something that uses IPA to inject “range” metadata on loads from globals?

Doing #2 would allow inferring that a global only has something in the range of 0..5, at least when the only stores are simple enough to analyze trivially.

-Chris

Sergei_Larin · September 30, 2015, 3:51pm

Chris, sorry for the delay with the response...

I deal with code that uses a linker scripts and explicitly set section attributes in the source (embedded environment) so most globals are firmly assigned to output sections. Preventing transformations on them is counterproductive, but in each case a new (copy, or modification) of the original GV is created, it needs to go to the same output section as did the original. It is even more important when something like this is assumed:

  // Retain the name of the old global variable. People who are debugging their
  // programs may expect these variables to be named the same.
  NewGV->takeName(GV);
  GV->eraseFromParent();

IR is sufficient for most simple cases when GlobalObject carries section assignment. I can always infer section being touched when dealing with any uses of it.

What IR is not designed to do is LTO in presence of a linker script, but this is another can of worms and we can discuss it in person at the dev meeting.

Meanwhile the remedy is trivial - just copy attributes from GV to NewGV every time NewGV can be explicitly present/used after the transformation - it will have no effect on cases when there is no section is set, and will allow some poor souls optimize embedded code a bit better