[PATCH] D66132: [CodeGen] Add `isSpeculativeExecutionForbidden()` to `TargetTransformInfo`

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D66132#1626871 <https://reviews.llvm.org/D66132#1626871>, @anton-afanasyev wrote:

> In D66132#1626811 <https://reviews.llvm.org/D66132#1626811>, @lebedev.ri wrote:
>
> > I feel like this is lacking words.
> >  What problem is this addressing?
> >  Is it correctness-one or performance?
> >  The current hook feels a little bit weird.
> >  Does it mean to completely ban speculative execution?
> >  If then there is a lot more places where it should be used, and not using it there may be surprising. 
> >  Or does it only mean to ban PRE? But then, what was the original problem?
> >  Or should it be per-MI hook?
> > etc
>
>
> The problem is discussed here: https://bugs.llvm.org/show_bug.cgi?id=42405
>  Per-MI hook already exists -- it is `hasSideEffect()`. But for particular targets setting this to `true` is undesirable, this breaks existing optimizations (though it could really have side effect, like `div` for aarch64.




> This change means to ban PRE at all, yes.

So give it (the hook) less misleading name?

Should this be only a hook, or should there be some `cl::opt` (and how should it iteract with the hook?
Asking for completeness only, i guess just a hook may be fine.

> @igorb could add something here.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66132/new/

https://reviews.llvm.org/D66132