[PATCH] Allow global structures to be vectorized

[Hal Finkel]

Renato,

Thanks for working on this...

If I'm reading this correctly, this will also handle cases like
for (i = 0; i < n; ++i)
  x[i+n] = x[i];
If not, why not?

----- Original Message -----
> From: "Arnold Schwaighofer" <aschwaighofer at apple.com>
> To: "Renato Golin" <renato.golin at linaro.org>
> Cc: "LLVM Commits" <llvm-commits at cs.uiuc.edu>, "Nadav Rotem" <nrotem at apple.com>, "Hal Finkel" <hfinkel at anl.gov>,
> "Chandler Carruth" <chandlerc at google.com>
> Sent: Wednesday, February 6, 2013 11:03:04 AM
> Subject: Re: [PATCH] Allow global structures to be vectorized
> 
> Hi Renato,
> 
> I don't think this is correct. You are asserting that: "Two geps will
> only alias if they have the same elements in the index list"?
> 
> -      if (WriteObjects.count(*it)) {
> -        DEBUG(dbgs() << "LV: Found a possible read/write reorder:"
> -              << **it <<"\n");
> -        return false;
> +      // We want to know the offset of GEPs, too
> +      std::pair<GEPOperator*, OffsetList> GEP = findGEPOperator(*I,
> Read, DL);
> +      if (GEP.first)
> +        Read = GEP.first;
> +      if (WriteObjects.count(Read)) {
> +        // Non-GEPs are directly related
> +        if (!GEP.first) {
> +          DEBUG(dbgs() << "LV: Found a possible read/write reorder:
> "
> +                << *Read <<"\n");
> +          return false;
> +        }
> +        // For GEPs Make sure they're not just different offsets
> +        OffsetList& WriteList = (*WriteObjects.find(Read)).second;
> +        OffsetList& ReadList = GEP.second;
> +        if (WriteList == ReadList) {
> +          DEBUG(dbgs() << "LV: Found a possible read/write reorder:
> "
> +                << *Read <<"\n");
> +          return false;
> +        }
> +        // TODO: Use vector size for further conflict detection?
> 
> This is not correct. To make any statement about aliasing of two GEPs
> you need to symbolically evaluate them (and their recursive inputs).
> Look at BasicAA::aliasGEP.
> 
> Also note, LLVM IR's semantic does *not* allow you to treat two GEP
> access like an index into a type tree, and assume if the paths are
> different you don't have an aliasing memory.
> 
> Just because you have:
> 
> struct {
>    int B[100];
>    int A[100];
> } S;
> 
> You cannot assume that
> 
> gep S, 0, 1, x
> gep S, 0, 0, y
> 
> don't alias for any x,y. LLVM's IR semantics allows you to walk
> beyond B[99]. Using gep S, 0, 0, 100 is perfectly legal in llvm ir.
> 
> 
> Similarly, you cannot assume that
> 
> gep S, 0, x, y
> gep S, 0, z, a
> 
> alias only if x ==z and y ==a.

This was the point I had tried to make previously; You might be able to use SE to prove disjointness for affine accesses, but in general you'll need runtime overlap checks.

 -Hal

> 
> Best,
> Arnold
> 
> 
> On Feb 6, 2013, at 10:17 AM, Renato Golin <renato.golin at linaro.org>
> wrote:
> 
> > Hi all,
> > 
> > Far from perfect, the patch adds an extra check on the offsets. I
> > may not be using the right containers, and the static function
> > findGEPOperator() is ill-placed, but that's the basic idea behind
> > checking for GEPs. Any comments are really appreciated.
> > 
> > Another thing I'd like to ask is about the vector size.
> > 
> > +  // TODO: Use vector size for further conflict detection?
> > 
> > Should I investigate if the offsets point to a reasonable distance
> > in memory, given the vector size?
> > 
> > If the write is on P and the read is on P+(2 x int) and the vector
> > size is (4 x int), they do alias, even though the GEP is not on
> > the same element.
> > 
> > I also may have to check that the address space of both pointers is
> > the same, and possibly add this to the ValueOffsetMap container...
> > or e may assume that address spaces will never overlap on
> > reasonable vector sizes (ie, they're not contiguous on at least 1
> > vector distance).
> > 
> > cheers,
> > --renato
> > <global_vectorize.patch>
> 
>