[PATCH] Promote GlobalMerge pass to a ModulePass to ease reuse

Tue May 14 10:21:35 PDT 2013

Hi Eric,

Thanks for having a look.
You confirm that my thoughts: there is not an easy fix and it is an interesting problem :).

I will try to spend more time looking into the debug information framework to go along with you.

Thanks again,

-Quentin

On May 14, 2013, at 10:08 AM, Eric Christopher <echristo at gmail.com> wrote:

> Hi Quentin,
> 
> I apologize for the delay, I wanted to do more of a look, but that
> didn't end up happening. I believe your summary is correct. I think
> the problem is that information is started for debug info via
> beginModule so you've got a similar problem there. It might end up
> being a long thread to unravel all of it, but that's my best guess at
> the moment. Other than the interface with the machine information and
> associated setup I'm not seeing anything that's screaming "must be in
> beginModule".
> 
> Happy to look into it with you as you go along etc.
> 
> -eric
> 
> 
> On Fri, Apr 19, 2013 at 11:08 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>> Hi Eric,
>> 
>> I’ve looked closer on the debug information were generated for the merged
>> variables.
>> It appeared that at the moment GlobalMerge has to perform its modifications
>> in doInitialization, because it relies on how the global variables will be
>> interpreted in DwarfDebug::beginModule which is called from the
>> doInitialization of the AsmPrinter pass.
>> More specifically, CompileUnit::createGlobalVariableDIE has a special case
>> for MergedGlobals:
>> […]
>> if (const ConstantExpr *CE = getMergedGlobalExpr(N->getOperand(11)))
>> 
>> After a (very) quick look to the debug interfaces, I didn’t see a way to
>> reproduce this behavior a posteriori.
>> 
>> Any ideas?
>> 
>> Thanks,
>> 
>> -Quentin
>> 
>> On Apr 17, 2013, at 9:31 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>> Thanks Eric.
>> 
>> Do you think it would be possible/desirable to fix the debug information
>> within GlobalMerge?
>> 
>> Currently GlobalMerge pass is added to the pass manager in code that is ARM
>> specific, thus, I don’t have much latitude to move it. In particular, if I
>> move it at the earliest I can in target specific code (i.e., using
>> addIRPasses) I didn’t reproduce the old behavior.
>> 
>> -Quentin
>> 
>> On Apr 16, 2013, at 7:18 PM, Eric Christopher <echristo at gmail.com> wrote:
>> 
>> On Tue, Apr 16, 2013 at 5:51 PM, Jim Grosbach <grosbach at apple.com> wrote:
>> 
>> 
>> On Apr 16, 2013, at 5:48 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>> Oh, that’s true the pass is not run where it was running.
>> Thanks Jim.
>> 
>> I’ll have to check if we can reproduce the old behavior since the pass may
>> rely on running before the doInitialization of another pass.
>> 
>> Regarding the test cases, the debug information have changed.
>> MergedGlobals is no more referenced in the debug information. I’m fine with
>> that since it was not in the program initially (we give insight on something
>> the developer never wrote!), but it seems to be the expected behavior.
>> 
>> 
>> Interesting. Well, it’s entirely possible that expected != correct here. If
>> we’re referring back (correctly, of course) to the user’s variables and not
>> referencing the merged globals variable at all, that seems an improvement to
>> me. Eric, any thoughts on that?
>> 
>> 
>> The new code is, unfortunately, incorrect. The addresses of the
>> various objects are relocations for undefined symbols in the output
>> _x1, _x2, etc rather than offsets off of the new _MergedGlobal.
>> Basically those static local symbols no longer exist and all uses of
>> them in the program have been replaced.
>> 
>> -eric
>> 
>> 
>> -Jim
>> 
>> 
>> -Quentin
>> 
>> On Apr 16, 2013, at 5:31 PM, Jim Grosbach <grosbach at apple.com> wrote:
>> 
>> Hi Quentin,
>> 
>> Hmmmm…. Perhaps we should consider whether to move the pass to where it was
>> actually really running before. We’ll want to consider whether the old
>> behavior was a bug, though, and it really belongs where it’s actually
>> running now (later). A quick glance suggests that the test case may just
>> need to be updated (i.e., the new order is fine, just different). We should
>> understand a bit more of what’s going on before assuming that, though.
>> 
>> -Jim
>> 
>> On Apr 16, 2013, at 4:27 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>> Hi,
>> 
>> I’ve noticed that all the work performed by the GlobalMerge pass is done
>> within the doInitialization method.
>> It’s unclear to me if it is a valid design or not, because according to
>> http://llvm.org/docs/WritingAnLLVMPass.html#the-functionpass-class:
>> - On one hand:
>> FunctionPass subclasses are not allowed to:
>> […]
>> 3. Add or remove global variables from the current Module.
>> - On the other hand:
>> The doInitialization method is allowed to do most of the things that
>> FunctionPasses are not allowed to do.
>> 
>> What is clear however is: This design prevents other passes to take
>> advantage of this pass. Let’s say I write a pass A that creates some global
>> variables and I’d like to see these variables merged. Thus, I’ll add
>> GlobalMerge pass after A. This does not work unless I push all the work of
>> pass A into doInitialization…
>> 
>> The proposed patch turns GlobalMerge into a ModulePass and move all its work
>> in runOnModule method.
>> Although this patch does not intend to make any functionality change it
>> seems that the phase ordering implied by this design was expected!
>> 
>> Indeed, with this patch I have two make check failures related to debug
>> info:
>> test/CodeGen/ARM/2011-01-19-MergedGlobalDbg.ll:26:14: error: expected string
>> not found in input
>> ;CHECK-NEXT: .long __MergedGlobals
>>            ^
>> <stdin>:233:2: note: scanning from here
>> .long _x2
>> ^
>> --
>> and
>> test/CodeGen/ARM/2011-08-02-MergedGlobalDbg.ll:17:14: error: expected string
>> not found in input
>> ;CHECK-NEXT: .long __MergedGlobals
>>            ^
>> <stdin>:205:2: note: scanning from here
>> .long _x2
>> ^
>> —
>> 
>> Any ideas on how I can fix those?
>> 
>> -Quentin
>> <GlobalMergeToModulePass.patch>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130514/b5708aca/attachment.html>