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

[lkail]

Summary

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

[steven.zhang]

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]

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]

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?

[steven.zhang]

In D91120#2391105, @spatel wrote:

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

[lkail]

In D91120#2391105, @spatel wrote:

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]

Pre-committed test case and rebased.

[spatel]

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?

[uweigand]

In D91120#2399776, @spatel wrote:

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]

In D91120#2399901, @uweigand wrote:

In D91120#2399776, @spatel wrote:

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...

[spatel]

In D91120#2402335, @jonpa wrote:

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]

Removed the hook and added test cases for other arch.

[steven.zhang]

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]

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.

[RKSimon]

Please can you rebase after rG9374e7b1781f00 ?

[lkail]

Rebased on Simon's commit.

[RKSimon]

LGTM

[lkail]

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