[PATCH] D152430: [DAG] Peek through freeze when deciding whether we should convert setcc to math or not.

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12251
   // =>
   //   FREEZE(SETCC(X, CONST, Cond))
   // This is correct if FREEZE(X) has one use and SETCC(FREEZE(X), CONST, Cond)
----------------
deadalnix wrote:
> RKSimon wrote:
> > Is this fold the underlying problem? Every other freeze fold we have tries to push the freeze upward through the DAG, but this pulling it down?
> I'm not sure it is the underlying problem, but it is indeed part of the loop.
> 
> The underlying problem is that we want to combine setcc to math, unless the setcc is used in a brcond. The math is turned back into a setcc by the brcond in that case, which created an infinite loop and this is why `PreferSetCC` was introduced in the first place.
> 
> As to pushing the freeze up or down, I don't really have an opinion on the matter.
Reworded: the loop problem existed in the past and was fixed, but this transform defeated the previous fix in the presence of a freeze.

I do indeed think that there needs to be some thought put into how we want to handle freeze, as this doesn't look very solid to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152430