[llvm] r208934 - Implement global merge optimization for global variables.

[Jiangning Liu]

Hi Rafael,


2014-05-17 21:15 GMT+08:00 Rafael Espíndola <rafael.espindola at gmail.com>:

> > I don't think Nick is understanding this issue correctly as I mentioned
> in
> > my last reply, so I'm not sure if you are really agree with him. The
> issue
> > you mentioned below is actually completely different from Nick's point.
> > Refer to my answer below, anyway.
>
> The part about unnamed_addr, yes, but not the part about alignment,
> right? Things like getGlobalMergeAlignment do look out of place.
>
> >> Is
> >> this uncommon enough that checking for uses coming from the same
> >> function was not considered profitable?
> >
> >
> > I'm not sure if this could be profitable, because they are global
> variables
> > and we don't really know how they are being referenced in other modules.
> The
> > heuristic of considering the uses within a function for current module
> would
> > not probably give the same benefit to other modules, so I'm thinking the
> > heuristic strategy of merging them all together for current module would
> not
> > make big difference from the one of further checking uses within a
> function
> > for current module.
> >
> > I never say my optimization patch is commonly profitable, and this is why
> > they are under switches control. But for some specific cases, it would be
> > profitable, I believe In particular, for the case that global variables
> are
> > being heavily used in the module that are defining them.
>
> Well, we normally don't want to have too many use facing switches.
> What is the plan for removing this one? If variable concatenation (to
> avoid confusion with merging which causes the address to be the same)
> is profitable enough for ARM/AARCH64 that it is better than the
> inability to dead strip some code, then it should always be enabled.
> If not, we should figure out an heuristic for when to do it.
>

I'm not sure if I'm understanding your point correctly. There might be
several different functions in a module(file), and you are asking for
concatenating symbols being used in a given function, so how do we handle
multiple multiple functions? Can you clarify this? One solution I could
have is we use a heuristic rules to concatenating symbols in a given
function as possible as we could, but we still need to merge all symbols
all together into a single MergedGlobal symbol. Or you mean you want to
define multiple MergedGlobal symbols and map them to each function? If this
is the case, how would you handle the conflict among different functions?

If essentially you are proposing to remove the unused symbol of current
module in MergedGlobal variable. I think this is a good point to me,
because anyway it's useless on helping optimization current module.


>
> It should also be done really late. In particular, it should probably
> not run with -flto and instead be enabled in the LTO pipeline.
>

I don't understand your point here. What should be done late? Do you mean
the GlobalMerge pass should be done late? At present, this pass happens in
PreISel stage, so where do you want to put it?

This optimization itself doesn't rely on LTO at all. But at present ARM64
already has LTO solution using LOH, we are thinking it can be orthogonal
with this static global merge solution. Yes, LTO is useful, but it can
replace static solution, and a lot of end-users prefer to use static
solution only.

Previously you also mentioned getGlobalMergeAlignment is out of place, so
if you are talking about it early to to set alignment for GlobalMerge
variabls, then could you please let me know where you think we should put
it? I thought it was quite natural to set it at the place of generating
this GlobalMerge symbol in global merge pass.


> I also don't follow your objection to looking at the function bodies
> to decide when it is profitable. If a variable is external, codegen
> must produce the fully general access pattern. That is, the only code
> that benefits from concatenating global variables is the one in the
> same TU as the variables, right?
>

Oh, I think I didn't say I object to checking function bodies, and I only
said it's also a kind of heuristic just like my simple solution of not
checking function bodies. But I realized that excluding "dead symbol" is a
good idea I should follow.

Sorry what does TU stand for?

The benefit for AArch64 is to reduce the ADRP (get page address of global
symbol) and ADD instructions.
(1) If two symbols are merged in the same data structure, they can share
the same base address.
(2) the offset of symbols to the page address is less than a page, offset
can be encoded into instruction, and we will save ADD instruction, and this
is why we want to set alignment in global merge pass.

Hopefully I described the problem clearly.


> >> putting two variables that a function uses next to each other would
> >> also be a win there, no?
> >
> >
> > X86 and ARM/AArch64 have different address model, and this is because of
> the
> > so-called CSIC and RISC differences. X86 can directly encode global
> address
> > relocation into instructions, while ARM/AArch64 can only encode it in
> > load/store instructions. For AArch64, this is getting worse, because ADRP
> > instruction can only get the page address of global variable due to the
> > limitation of instruction encoding size (32-bit only), and we have to
> > introduce another add instruction to get the 64-bit address. This is why
> > this optimization is very meaningful to AArch64. I think you would be
> able
> > to understand this much better if you look into the very beginning part
> of
> > my patch review thread.
> >
> > Yes, putting two global variables that a function next each other would
> be a
> > win, but as I mentioned above this is also heuristic because we don't
> know
> > how global variables are being used by other modules at compilation time.
>
> It is heuristic. I was just pointing out that it is not fundamentally
> an ARM only thing.
>
> >
> > OK. Thanks for letting me know that. Hopefully my patch still can reuse
> your
> > new GlobalAlias solution. Looking at the bug tracker you pointed to me,
> it
> > seems to be a very old problem raised by Christ long time ago. I'm not
> sure
> > if you can really make it happen next week. I would be extremely
> > appreciative if you do.
>
> I think I can. The main change is already in. What is missing is just
> the adding offsets.
>

I noticed that you already sent out the patch of supporting alias offset,
so appreciate your quick patch, although I also noticed there is a long
discussion around your previous commit of removing ConstantExpr for alias.
:-) Personally I'm really hoping your alias offset support patch can go
into trunk as soon as possible, and then recover my global merge patch.

Thanks,
-Jiangning


>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140519/dba18559/attachment.html>