[Patch] Use address-taken to disambiguate global var and indirect accesses

[Nick Lewycky]

Shuxin Yang wrote:
> Hi,
>
> The attached patch is to take advantage of address-taken to disambiguate
> global
> variable and indirect memory accesses.

Thanks for this patch. This is the right approach for solving this AA 
problem.

Could I ask for the keyword to be worded "addr not taken" instead of 
"not addr taken"?

Does addr_not_taken imply unnamed_addr? I'm pretty sure it does. If so, 
please make sure that this works (see how readonly/readnone are handled 
-- readnone implies readonly).

I'm not convinced that you actually need to cache this result. You say 
that it isn't efficient to determine whether the address is taken on the 
fly. How bad is it if you just loop over the direct users of the global 
variable, and bail if you see anything that isn't a load or store? Yes, 
it would have false negatives on load/store of constantexpr GEPs. How 
big a problem is that really? If that really doesn't work, is it okay if 
you do a depth-first scan over uses, and bail on anything which isn't 
load/store/constant-gep/constant-bitcast?

I can totally believe that caching it is the way to go, but we should 
hard try to avoid it if possible.

--- lib/AsmParser/LLParser.cpp	(revision 192719)
+++ lib/AsmParser/LLParser.cpp	(working copy)
@@ -704,7 +704,7 @@
                             unsigned Linkage, bool HasLinkage,
                             unsigned Visibility) {
    unsigned AddrSpace;
-  bool IsConstant, UnnamedAddr, IsExternallyInitialized;
+  bool IsConstant, UnnamedAddr, IsExternallyInitialized, notAddrTaken;

Local variables start with a capital letter.

Index: docs/LangRef.rst
===================================================================
--- docs/LangRef.rst	(revision 192719)
+++ docs/LangRef.rst	(working copy)
@@ -514,6 +514,9 @@
  ``@llvm.used``. This assumption may be suppressed by marking the
  variable with ``externally_initialized``.

+If a global variable dose not have its address taken, it will be optionally
+flagged ``notaddrtaken``.


I think this should be merged with the discussion about unnamed_addr. 
The thing about unnamed_addr is that the address may still be taken and 
stored somewhere, as long as all its users are ultimately load and store 
of ptr + offset. addrnottaken is stricter in that you may not take its 
address as all.

Proposed text:

Global variables can be marked with ``addrnottaken`` which indicates 
that no other copies of this pointer's address exist at any time in the 
program. They may also be marked with ``unnamed_addr`` which indicates
that the address may be taken and copies of it made, as long as it is 
only used to access the content and the address itself is not observed. 
Constants marked with either of these can be merged with other constants 
if they have the same initializer. Note that a constant with significant 
address *can* be merged with a ``unnamed_addr`` constant, the result 
being a constant whose address is significant.


The proposed text is a bit weak, what does "no other copies" mean, that 
I can't create an SSA variable with the address? Or that I can't put a 
pointer to it in memory? (We know what we want it to mean, but it needs 
to be clear to people who aren't familiar with the problem you're solving.)

Nick

>
> The motivation
> ===========
> I was asked to investigate the problem where the static variable is not
> hoisted as
> loop invariant:
>
> ---------------
> static int xyz;
> void foo(int *p) {
> for (int i = 0; i < xyz; i++)
> *p++ = ....
> }
> -----------------
>
> The compiler dose have a concept call "addr-capture". However, I don't
> think it can
> be used for disambiguate global variable and indirect access. The
> reasons is that
> a variable dose not have its address *CAPTURED*, dose not necessarily
> mean this variable
> cannot be indirectly accessed.
>
> So, I rely on "address taken"
>
> How it works
> ========
> 1. In globalopt, when a global var is identified as not-addr-taken,
> cache the result
> to GlobalVariable::notAddrTaken.
>
> 2. In alias-analyzer, supposed the mem-op involved are m1 and m2. Let o1
> and o2
> be the "object" (obtained via get_underlying_object() of m1 and m2
> respectively.
>
> if O1 != O2 && one of the them are global-variable without address taken,
> then m1 and m2 are disjointed access.
>
> Misc:
> =========
> Note that I *cache* the result of not-addr-taken. Unlike function, it is
> far more expensive
> to figure out if a globalvar has its address taken or not. So, it is not
> appropriate to analyze
> the address-taken on the fly.
>
> On the other hand, I personally think not-addr-taken flag is almost
> maintenance free.
> (FIXME) Only few optimizer could make a not-addr-taken variable become
> addr-taken (say, outlining),
> and I don't think currently we have such passes (FIXME again!). In case
> such rare cases take place,
> it is up to the pass the to reset the not-addr-taken flags.
>
> Of course, a variable previously considered addr-taken may later on
> proved to be not-addr-taken.
> In that case, compiler dose not have to update it -- it is
> conservatively correct.
>
> Performance impact
> =============
> Measured on an oldish Mac Tower with 2x 2.26Ghz Quad-Core Xeon. Both
> base-line and
> the change are measured for couple of times. I did take a look of why
> Olden/power is sped up --
> the loads of static variable "P" and "Q" are promoted in many places. I
> have not yet got chance
> to investigate why the speedup to pairlocalalign with O3 disappear in
> O3+LTO.
>
>
> o. test-suite w/ O3:
> -------------------
> Benchmarks/Olden/power/power 1.6129 1.354 -16.0518321036642
> Benchmarks/mafft/pairlocalalign 31.4211 26.5794 -15.4090722476298
> Benchmarks/Ptrdist/yacr2/yacr2 0.881 0.804 -8.74006810442678
>
> o. test-suite w/ O3 + LTO
> -------------------------
> Benchmarks/Olden/power/power 1.6143 1.3419 -16.8741869540978
> Applications/spiff/spiff 2.9203 2.849 -2.44152997979659
>
> o. spec2kint w/ O3+LTO
> ----------------------
> bzip2 75.02 73.92 -1.4
>
>
> Thanks
> Shuxin
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits