Page MenuHomePhabricator

[DAGCombine][PowerPC] Convert negated abs to trivial arithmetic ops
ClosedPublic

Authored by lkail on Nov 9 2020, 9:28 PM.

Details

Summary

This patch converts 0 - abs(x) to Y = sra (X, size(X)-1); sub (Y, xor (X, Y)) for better codegen.

Diff Detail

Event Timeline

lkail created this revision.Nov 9 2020, 9:28 PM
lkail requested review of this revision.Nov 9 2020, 9:28 PM
steven.zhang added inline comments.Nov 11 2020, 3:52 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9348

Should we fold it into sub (0, ABS()) if ABS is legal ? And how about the VSELECT ? And it also surprise me that, there isn't pattern here for the ABS(). If it has, your code might merge with that one.

RKSimon added inline comments.Nov 12 2020, 2:35 AM
llvm/test/CodeGen/PowerPC/select.ll
133 ↗(On Diff #304038)

Commit these new tests to trunk with current codegen then rebase this patch to show the codegen diff?

spatel added a subscriber: nikic.Nov 12 2020, 5:38 AM

Note that we are planning to canonicalize IR to the abs() intrinsic - D87188.
Assuming that comes through relatively soon (cc @nikic), should we hold off adding what would probably become dead code in the backend?

Note that we are planning to canonicalize IR to the abs() intrinsic - D87188.
Assuming that comes through relatively soon (cc @nikic), should we hold off adding what would probably become dead code in the backend?

That is good news. But we still need to combine the sub(0, abs) to sradi/xor/sub as it saves one instruction.
What we have now with D87188:

0:	76 fe 64 7c 	sradi   r4,r3,63
4:	14 22 63 7c 	add     r3,r3,r4
8:	78 22 63 7c 	xor     r3,r3,r4
c:	d0 00 63 7c 	neg     r3,r3

And it can be improved to:

0:	76 fe 60 7c 	sradi   r0,r3,63
4:	78 02 63 7c 	xor     r3,r3,r0
8:	50 00 63 7c 	subf    r3,r3,r0

Note that we are planning to canonicalize IR to the abs() intrinsic - D87188.
Assuming that comes through relatively soon (cc @nikic), should we hold off adding what would probably become dead code in the backend?

Thanks for the info. Let me apply D87188 and see what will be going on. After D87188 is landed, I'll update the patch accordingly if necessary.

lkail updated this revision to Diff 305049.Nov 13 2020, 12:14 AM

Pre-committed test case and rebased.

lkail updated this revision to Diff 305700.Nov 17 2020, 1:48 AM
lkail retitled this revision from [DAGCombine][PowerPC] Fold negated abs to avoid generating `select_cc` to [DAGCombine][PowerPC] Convert negated abs to trivial arithmetic ops.
lkail edited the summary of this revision. (Show Details)

It seems strange that we would need to add a TLI hook to avoid regressing a target that has abs/nabs support in the ISA.
If I'm seeing it correctly, SystemZ relies on ISD::ABS being expanded and then pattern-matching the expansion back to lpgr for example. Could we change that target to declare ISD::ABS as legal and adjust the tablegen matching for it?

It seems strange that we would need to add a TLI hook to avoid regressing a target that has abs/nabs support in the ISA.
If I'm seeing it correctly, SystemZ relies on ISD::ABS being expanded and then pattern-matching the expansion back to lpgr for example. Could we change that target to declare ISD::ABS as legal and adjust the tablegen matching for it?

That SystemZ code pre-dates the existence of the common ISD::ABS code by four years or so. I guess now that common code support ISD::ABS, we can change our back-end to use it instead of our own. @jonpa , can you have a look?

jonpa added a comment.Nov 18 2020, 4:07 AM

It seems strange that we would need to add a TLI hook to avoid regressing a target that has abs/nabs support in the ISA.
If I'm seeing it correctly, SystemZ relies on ISD::ABS being expanded and then pattern-matching the expansion back to lpgr for example. Could we change that target to declare ISD::ABS as legal and adjust the tablegen matching for it?

That SystemZ code pre-dates the existence of the common ISD::ABS code by four years or so. I guess now that common code support ISD::ABS, we can change our back-end to use it instead of our own. @jonpa , can you have a look?

Yes, this seems to work: https://reviews.llvm.org/D91697. The new TLI hook is then not needed...

Yes, this seems to work: https://reviews.llvm.org/D91697. The new TLI hook is then not needed...

Nice - thank you for the fast fix!

lkail updated this revision to Diff 306284.Nov 18 2020, 6:43 PM

Removed the hook and added test cases for other arch.

steven.zhang added inline comments.Nov 18 2020, 6:55 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3221

Hmm, it looks strange to me that we have to add these two ops into worklist manual. Isn't it added into worklist automatically as far as the parent node (SUB) returned ? Please correct me if I misunderstand this.

lkail added inline comments.Nov 18 2020, 7:24 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3221

Good catch. I think the combine loop is supposed to add direct child nodes to the worklist. Since AddToWorklist uniques nodes added, I think it's also ok to call it manually. Another question arises, can we always rely on the behavior of the combine loop adding direct child node to the worklist automatically.

Please can you rebase after rG9374e7b1781f00 ?

lkail updated this revision to Diff 306595.Nov 19 2020, 9:31 PM

Rebased on Simon's commit.

This revision is now accepted and ready to land.Nov 20 2020, 12:45 AM

Removed parent revision, since this patch doesn't rely on opt's abs canonicalization.

This revision was landed with ongoing or failed builds.Nov 24 2020, 1:43 AM
This revision was automatically updated to reflect the committed changes.