[PATCH] D90361: Prevent LICM and machineLICM from hoisting convergent operations

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D90361#2364797 <https://reviews.llvm.org/D90361#2364797>, @qcolombet wrote:

> Hi @foad, hi @arsenm
>
>> This is allowed by the current definition. See D85603 <https://reviews.llvm.org/D85603>
>
> I was gong to say that @xiaoqing_wu's patch looks good to me, but I didn't think this was allowed by the current definition, but you're right it looks like it is.
> Now, in practice is this something that works for you guys?
>
> I would rather that we fix that asap than pinning that on the new convergence revamp, but if the current definition is okay for some users, I guess we'll have to explore a more target specific way to describe what is and what is not okay to hoist.
>
> What do you think?
>
> Cheers,
> -Quentin

This moves in the more conservative direction more in line with the new definition. It's good for some convergent cases, but not all. For the machine level, I don't think we have an answer for the equivalent of the convergence operands yet.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1164
+    if (CI->hasFnAttr(Attribute::Convergent))
+      return false;
+
----------------
isConvergent instead of directly checking the attribute


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90361