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

[Eric Christopher]

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
>
>