This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner]: Add transform (and/or (icmp eq/ne (A, C)), (icmp eq/ne (A, -C))) -> (icmp eq/ne (ABS A), ABS(C))
ClosedPublic

Authored by goldstein.w.n on Jan 25 2023, 10:13 PM.

Details

Summary

This can be beneficial if there is a fast ABS (For example with X86
vpabs) or if there is a dominating ABS(A) in the DAG.

Note C is constant so ABS(C) is just a constant.

Alive2 Links:
EQ: https://alive2.llvm.org/ce/z/829F-c
NE: https://alive2.llvm.org/ce/z/tsS8bU

Diff Detail

Event Timeline

goldstein.w.n created this revision.Jan 25 2023, 10:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2023, 10:13 PM
goldstein.w.n requested review of this revision.Jan 25 2023, 10:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2023, 10:13 PM

Wondering if I should merge this commit with D142344 or would that be too much?

goldstein.w.n added inline comments.Jan 25 2023, 10:20 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
5677

I don't think this is an issue, but it looks "close" to causing an infinite loop.
In X86ISelLowering.cpp from D142345 there is a transform to covert scalar (icmp eq/ne (ABS A), C) -> the logicop/setcc pattern here that we are about to transform back to the ABS pattern. The (icmp eq/ne (ABS A), C) is guarded behind hasOneUse and this is guarded behind doesNodeExist so I believe its safe (after this transform the node will never have only one use), but whoever reviews should probably double check the reasoning/etc.

pengfei accepted this revision.Feb 6 2023, 6:28 PM

LGTM.

This revision is now accepted and ready to land.Feb 6 2023, 6:28 PM