Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Authored by deadalnix on Jun 8 2023, 2:26 AM.

Details

Summary

Because later tranform push the freeze through the setcc, we end up having to reconstruct the setcc, which can cause infinite loops.

Diff Detail

Event Timeline

deadalnix created this revision.Jun 8 2023, 2:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2023, 2:26 AM
deadalnix requested review of this revision.Jun 8 2023, 2:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2023, 2:26 AM
RKSimon added inline comments.Jun 8 2023, 2:45 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12251

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?

deadalnix added inline comments.Jun 9 2023, 6:02 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12251

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.

deadalnix added inline comments.Jun 9 2023, 6:14 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12251

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.

nikic added inline comments.Jun 9 2023, 8:12 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12251

Possible alternative: https://reviews.llvm.org/D152544

Please can you abandon this and rebase/parent D127115 to D152544

deadalnix abandoned this revision.Jun 12 2023, 4:31 AM