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

[Eric Christopher]

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