llvm dev - May 2017 - -speculative-execution moving load before store

Hi,

A few days ago I stumbled upon a problem where SpeculativeExecution
changed the order of a load and a store to the same address.

I wrote

  https://bugs.llvm.org//show_bug.cgi?id=32964

about it but no response there so far.

In the input we have

   store i8 0, i8* @i
   %.pre = load i8, i8* @i

and then in the output the load is moved so it's before the store which 
clearly makes it load the wrong value.

isSafeToSpeculativelyExecute happily says "true" for the load, and
there
is a comment on isSafeToSpeculativelyExecute saying

   /// This method can return true for instructions that read memory;
   /// for such instructions, moving them may change the resulting value.

And that is indeed the case in PR32964.

How is this supposed to work?

I started seeing this after the commit

  SpeculativeExecution: Stop using whitelist for costs

Before that, e.g. a load got the cost UINT_MAX which I suppose disabled 
hoisting, but since the commit TTI.getUserCost(I) is used for all 
instructions.

Regards,
Mikael

On 5/9/2017 11:37 PM, Mikael Holmén via llvm-dev wrote:
> Hi,
>
> A few days ago I stumbled upon a problem where SpeculativeExecution
> changed the order of a load and a store to the same address.
>
> I wrote
>
>  https://bugs.llvm.org//show_bug.cgi?id=32964
>
> about it but no response there so far.
>
> In the input we have
>
>   store i8 0, i8* @i
>   %.pre = load i8, i8* @i
>
> and then in the output the load is moved so it's before the store 
> which clearly makes it load the wrong value.
>
> isSafeToSpeculativelyExecute happily says "true" for the load,
and
> there is a comment on isSafeToSpeculativelyExecute saying
>
>   /// This method can return true for instructions that read memory;
>   /// for such instructions, moving them may change the resulting value.
>
> And that is indeed the case in PR32964.
>
> How is this supposed to work?
Passes are supposed to check whether the moved instruction actually 
produces the right result.  As a short-term fix, you can just add a 
check for mayReadFromMemory().  If you want to do something fancier, 
MemorySSA provides a convenient interface to check aliasing.

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project

This looks like it's due to r301950, which has today been reverted in
r302640. Can you try with the latest trunk?

Regardless, there seems to be an underlying bug exposed.

On 10 May 2017 at 07:37, Mikael Holmén via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> A few days ago I stumbled upon a problem where SpeculativeExecution
> changed the order of a load and a store to the same address.
>
> I wrote
>
>  https://bugs.llvm.org//show_bug.cgi?id=32964
>
> about it but no response there so far.
>
> In the input we have
>
>   store i8 0, i8* @i
>   %.pre = load i8, i8* @i
>
> and then in the output the load is moved so it's before the store which
> clearly makes it load the wrong value.
>
> isSafeToSpeculativelyExecute happily says "true" for the load,
and there is
> a comment on isSafeToSpeculativelyExecute saying
>
>   /// This method can return true for instructions that read memory;
>   /// for such instructions, moving them may change the resulting value.
>
> And that is indeed the case in PR32964.
>
> How is this supposed to work?
>
> I started seeing this after the commit
>
>  SpeculativeExecution: Stop using whitelist for costs
>
> Before that, e.g. a load got the cost UINT_MAX which I suppose disabled
> hoisting, but since the commit TTI.getUserCost(I) is used for all
> instructions.
>
> Regards,
> Mikael
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

(staring at this pass, i'm not sure it's supposed to work in all cases
on
all processors.
If it is, it ... uh, needs help :P)


On Wed, May 10, 2017 at 12:00 PM, Amara Emerson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> This looks like it's due to r301950, which has today been reverted in
> r302640. Can you try with the latest trunk?
>
> Regardless, there seems to be an underlying bug exposed.
>
> On 10 May 2017 at 07:37, Mikael Holmén via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > Hi,
> >
> > A few days ago I stumbled upon a problem where SpeculativeExecution
> > changed the order of a load and a store to the same address.
> >
> > I wrote
> >
> >  https://bugs.llvm.org//show_bug.cgi?id=32964
> >
> > about it but no response there so far.
> >
> > In the input we have
> >
> >   store i8 0, i8* @i
> >   %.pre = load i8, i8* @i
> >
> > and then in the output the load is moved so it's before the store
which
> > clearly makes it load the wrong value.
> >
> > isSafeToSpeculativelyExecute happily says "true" for the
load, and there
> is
> > a comment on isSafeToSpeculativelyExecute saying
> >
> >   /// This method can return true for instructions that read memory;
> >   /// for such instructions, moving them may change the resulting
value.
> >
> > And that is indeed the case in PR32964.
> >
> > How is this supposed to work?
> >
> > I started seeing this after the commit
> >
> >  SpeculativeExecution: Stop using whitelist for costs
> >
> > Before that, e.g. a load got the cost UINT_MAX which I suppose
disabled
> > hoisting, but since the commit TTI.getUserCost(I) is used for all
> > instructions.
> >
> > Regards,
> > Mikael
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170510/9895bcc5/attachment.html>

On 05/10/2017 09:00 PM, Amara Emerson wrote:
> This looks like it's due to r301950, which has today been reverted in
> r302640. Can you try with the latest trunk?
>
Yep after the revert it works again.

Chandler (who reverted r301950) updated the original review for r302640 
with the findings: https://reviews.llvm.org/D24544

/Mikael
> Regardless, there seems to be an underlying bug exposed.
>
> On 10 May 2017 at 07:37, Mikael Holmén via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> Hi,
>>
>> A few days ago I stumbled upon a problem where SpeculativeExecution
>> changed the order of a load and a store to the same address.
>>
>> I wrote
>>
>>  https://bugs.llvm.org//show_bug.cgi?id=32964
>>
>> about it but no response there so far.
>>
>> In the input we have
>>
>>   store i8 0, i8* @i
>>   %.pre = load i8, i8* @i
>>
>> and then in the output the load is moved so it's before the store
which
>> clearly makes it load the wrong value.
>>
>> isSafeToSpeculativelyExecute happily says "true" for the
load, and there is
>> a comment on isSafeToSpeculativelyExecute saying
>>
>>   /// This method can return true for instructions that read memory;
>>   /// for such instructions, moving them may change the resulting
value.
>>
>> And that is indeed the case in PR32964.
>>
>> How is this supposed to work?
>>
>> I started seeing this after the commit
>>
>>  SpeculativeExecution: Stop using whitelist for costs
>>
>> Before that, e.g. a load got the cost UINT_MAX which I suppose disabled
>> hoisting, but since the commit TTI.getUserCost(I) is used for all
>> instructions.
>>
>> Regards,
>> Mikael
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev