[PATCH] D24544: SpeculativeExecution: Stop using whitelist for costs

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D24544#750762, @chandlerc wrote:

> In https://reviews.llvm.org/D24544#546434, @reames wrote:
>
> > I'd suggest looking over the isSafeToSpeculate code carefully.  If we had any bugs there, this whitelist might have been helping to cover over them in practice.  In particular, the default: return true; case in that function looks suspicious.  You might want to convert that into a fully covered switch and cross reference the two lists to make sure this change isn't accidentally semantic.
>
>
> Perhaps unsurprisingly, this review comment proved prophetic.
>
> This patch causes us to "miscompile" some CUDA kernels.
>
> That said, LLVM may not be at fault as PTXAS has known bugs that miscompile valid PTX programs, so I'm going to try to somehow produce a test case that shows what is going on here.


After some basic investigation, this patch is really broken.

`isSafeToSpeculativelyExecute` doesn't actually mean it is safe to hoist something. You need a lot more to guarantee that:

- Does it read from memory?
  - Then you can't hoist it over a clobbering store
  - You can't hoist it over a function call which could map that memory
  - You can't hoist it over a synchronization point with another thread that could map that memory (or be a clobbering store)

I haven't even thought hard about what the full, safe predicate is here. But I suspect this pass needs to look very closely at what existing, well tested LLVM passes use to do hoisting, or it needs to continue to use a whitelist.

For now, I'm reverting this patch because I don't even know how many test cases need to be written here.

Also, if the whitelist is removed, the test against `UINT_MAX` shoudl be as well.

> In the meantime, it sure would be nice to do as Philip suggested and make that switch covering so that we don't just have the default be safe. Or at least make the default be unsafe, which seems ... far better.

And we should still do this as I don't really have a lot of confidence in that routine as-is.


https://reviews.llvm.org/D24544