[PATCH] D37289: [X86] Speculatively load operands of select instruction

[Lama via Phabricator via llvm-commits]

lsaba added a comment.

In https://reviews.llvm.org/D37289#857668, @spatel wrote:

> In https://reviews.llvm.org/D37289#857652, @lsaba wrote:
>
> > In https://reviews.llvm.org/D37289#857637, @spatel wrote:
> >
> > > In https://reviews.llvm.org/D37289#857435, @lsaba wrote:
> > >
> > > > In https://reviews.llvm.org/D37289#857007, @spatel wrote:
> > > >
> > > > > I didn't look at the implementation, but why is it safe to speculate loads in these tests? I can create an example where one of the pointers in the select is unmapped, so speculating that load will crash in the general case.
> > > >
> > > >
> > > > The implementation handles a specific case where both operands of the select are GEPs into elements of the same struct, correct me if i'm wrong but this should be safe
> > >
> > >
> > > I don't see how being elements of one struct changes anything. Just because one pointer is dereferenceable does not make the other dereferenceable? You would need 'dereferenceable' metadata or some other means to know that either load is safe to hoist ahead of the select.
> > >
> > > You're proposing this transform as an x86-specific pass, so maybe I'm missing something. Is there some feature of x86 that makes speculating the load safe? I'm guessing no because I tested the example I was thinking of on x86, and this transform crashes there.
> >
> >
> > This is the transformation I am interested in doing:
> >
> > struct S {
> >
> >   int a;
> >   int b;
> >
> > }
> >
> > from:
> >
> > foo (S* s, int x) {
> >
> >    int c;
> >    if (x) 
> >      c = s->a;
> >    else
> >     c = s->b;
> >   }
> >   
> >
> > to:
> >  foo (S* s, int x) {
> >
> >    int c1= s->a;
> >    int c2 = a->b;
> >    c = x? c1 : c2;
> >   }
> >   
> >
> > I am assuming this transformation is legal in C for the given struct with the given types since the entire struct is allocated,  is my assumption wrong? the idea is to limit the pass to these cases ( I am uploading a patch to limit the current implementation more)
>
>
> Yes, I think your assumption is wrong. It's contrived, but consider this possibility based on the current version of the patch:
>
>   #include <stdlib.h>
>   typedef struct S {
>     char padding[4088]; // not necessary, but might it make it easier for GuardMalloc or valgrind to see the bug
>     struct S *p1;
>     struct S *p2;
>   } S;
>  
>   S *Sptr;
>  
>   void init() {
>     Sptr = malloc(4096); // sorry, p2 - no space for you
>     Sptr->p1 = "1239"; // crazy, but just to prove a point
>   }
>  
>
>
> When input to a wrapper test similar to the test in this patch, this is safe without this transform, but crashes after. You need something to tell you the loads are dereferenceable (and whatever that information is will not be x86-specific, so there's no need to make an x86 pass for it).


you're right, this is problematic. Thanks, I will try to limit this to cases where it is safe to do this


https://reviews.llvm.org/D37289