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

Paths

Table of Contentst

[PowerPC] Replace PPCISD::VABSD cases with generic ISD::ABDU(X,Y) node
ClosedPublic
Actions

Authored by RKSimon on Jan 22 2023, 10:13 AM.

Details

Reviewers

nemanjai
Esme
steven.zhang

Commits

rG8757ce490130: [PowerPC] Replace PPCISD::VABSD cases with generic ISD::ABDU(X,Y) node

Summary

A move towards using the generic ISD::ABDU nodes on more backends

Also support ISD::ABDS for v4i32 types using the existing signbit flip trick

PowerPC has a select(icmp_ugt(x,y),sub(x,y),sub(y,x)) -> abdu(x,y) that I intend to move to DAGCombiner in a future patch.

I don't think the PPCISD::VABSD(X,Y,1) v4i32 combine was legal (at least alive2 didn't think so) - so I've removed it.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

RKSimon created this revision.Jan 22 2023, 10:13 AM

Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2023, 10:13 AM

Herald added subscribers: shchenz, kbarton, hiraditya. · View Herald Transcript

RKSimon requested review of this revision.Jan 22 2023, 10:13 AM

Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2023, 10:13 AM

Harbormaster completed remote builds in B209239: Diff 491183.Jan 22 2023, 12:16 PM

Thanks for updating this. I am just curious, do the combines you removed just end up getting combined to a VSELECT?

In D142313#4075615, @nemanjai wrote:

Thanks for updating this. I am just curious, do the combines you removed just end up getting combined to a VSELECT?

@dmgreen added them to DAGCombiner as part of adding the generic abds/abdu nodes

What should be done with the PPCISD::VABSD nodes? They can only be used for the PPCISD::VABSD(X, 1) case now - should they be simplified/renamed?

RKSimon added inline comments.Jan 26 2023, 9:31 AM

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
17667–17668	I'm wondering if this is actually correct? If there had been SIGN_EXTEND/SIGN_EXTEND_VECTOR_INREG equivalent nodes to the previous ABDU pattern then this would be ABSD. https://alive2.llvm.org/ce/z/k8wpeF

RKSimon mentioned this in rG846ec9092483: [PowerPC] ppc64-P9-vabsd.ll - add some basic ISD::ABDS test coverage.Jan 27 2023, 3:12 AM

Adds support for ISD::ABDS for v4i32 types using the existing signbit flip trick and remove the PPCISD::VABSD node type entirely

I've tentatively removed the "(abs (sub a, b) --> (vabsd a b 1))" fold as afaict its not correct: https://alive2.llvm.org/ce/z/jc2hLU

Harbormaster completed remote builds in B210399: Diff 492800.Jan 27 2023, 12:00 PM

ping?

ping2?

llvm/test/CodeGen/PowerPC/ppc64-P9-vabsd.ll
1249 ↗	(On Diff #492800)	if we replace this with sub nsw then the fold is legal

LGTM. Thanks for cleaning this up and sorry about the delay in reviewing it.

llvm/test/CodeGen/PowerPC/ppc64-P9-vabsd.ll
1249 ↗	(On Diff #492800)	I suppose the test case then becomes equivalent to `sub_absv_32()` other than the use of the `vmaxsw` intrinsic. I think we can just add the flag so the test case shows the desired code-gen.

This revision is now accepted and ready to land.Feb 24 2023, 3:36 AM

RKSimon mentioned this in rGab76f5865f21: [PPC] Fix abs(sub(x,y)) -> abs(x,y) tests.Feb 25 2023, 12:03 PM

This revision was landed with ongoing or failed builds.Feb 25 2023, 12:17 PM

Closed by commit rG8757ce490130: [PowerPC] Replace PPCISD::VABSD cases with generic ISD::ABDU(X,Y) node (authored by RKSimon). · Explain Why

This revision was automatically updated to reflect the committed changes.

RKSimon added a commit: rG8757ce490130: [PowerPC] Replace PPCISD::VABSD cases with generic ISD::ABDU(X,Y) node.

Hi @RKSimon

The ABS(SUB(X,Y)) -> PPCISD::VABSD(X,Y,1) v4i32 combine wasn't legal (https://alive2.llvm.org/ce/z/jc2hLU) - so I've removed it, having already added the legal sub nsw tests equivalent.

For case where X and Y are both zero extended from other values, the original transformation looks still correct. See https://alive2.llvm.org/ce/z/LWaEEg.