Page MenuHomePhabricator

[DAG] Fold (mul(abs(x),abs(x))) -> (mul(x,x)) (PR39476)
AbandonedPublic

Authored by RKSimon on May 3 2020, 10:19 AM.

Details

Summary

This patch adds support for discarding integer absolutes from self-multiplications.

Alive2: http://volta.cs.utah.edu:8080/z/rwcc8W

As part of this I've pulled out various absolute pattern matching code snippets we have and moved them into a MatchABS helper, which returns the source operand on success. We can add other ABS patterns here as required (e.g. PR45691).

NOTE: We shouldn't move the select(0>X,0-X,X) pattern as this is always canonicalized to a branchless pattern in visitVSELECT even if ISD::ABS isn't legal.

Diff Detail

Event Timeline

RKSimon created this revision.May 3 2020, 10:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2020, 10:19 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Echoing what @craig.topper asked, do we somehow end up with that pattern after instcombine had a chance to do this fold?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
5059

Precommit NFC cleanup?

lebedev.ri added inline comments.May 3 2020, 11:14 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3527–3530

This LG

RKSimon marked an inline comment as done.May 3 2020, 11:42 AM

Echoing what @craig.topper asked, do we somehow end up with that pattern after instcombine had a chance to do this fold?

Where did he ask that? https://godbolt.org/z/4vIPwf (from PR39476) shows that this IR currently comes through to the backend.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
5059

np - if people are happy with the patch I'll pull this out as a NFC precommit

Echoing what @craig.topper asked, do we somehow end up with that pattern after instcombine had a chance to do this fold?

Where did he ask that?

https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200427/776590.html

https://godbolt.org/z/4vIPwf (from PR39476) shows that this IR currently comes through to the backend.

Now that we handle it in instcombine, do we still want dagcombine fold?

RKSimon abandoned this revision.Wed, May 6, 3:32 AM
RKSimon marked an inline comment as not done.

I'm going to abandon this (and I will remove the test cases from combine-mul.ll) - if we find a case where we need this in DAG then its trivial to reimplement.