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

Mon May 13 10:21:57 PDT 2013

Ping?

Thanks,

-Quentin

On May 2, 2013, at 9:59 AM, Quentin Colombet <qcolombet at apple.com> wrote:

> Hi Eric,
> 
> I know you are busy and I appreciate you are planning to have a look on that.
> A question though, when do you think you will get a chance to look at that?
> 
> Thanks,
> 
> -Quentin
> 
> On Apr 23, 2013, at 9:19 AM, Eric Christopher <echristo at gmail.com> wrote:
> 
>> I haven't forgotten :)
>> 
>> I'll try to get you a reply tomorrow.
>> 
>> -eric
>> 
>> On Tue, Apr 23, 2013 at 5:16 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>> Ping?
>>> 
>>> -Quentin
>>> 
>>> On 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
>>> 
>>> 
>>> _______________________________________________
>>> 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/20130513/0dc868dc/attachment.html>