-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

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

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.

(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)

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