Page MenuHomePhabricator

[X86] Add basic vector handling for ISD::ABDS/ABDU (absolute difference) nodes
ClosedPublic

Authored by RKSimon on Jan 21 2023, 11:28 AM.

Details

Summary

I'm intending to add generic legalization in the future, but for now I've added basic support to targets that have the necessary MIN/MAX support to expand to SUB(MAX(X,Y),MIN(X,Y)).

This exposed a couple of issues with the DAG combines - in particular we need to catch trunc(abs(sub(ext(x),ext(y)))) patterns earlier before the SSE/AVX vector trunc expansion folds trigger.

Diff Detail

Event Timeline

RKSimon created this revision.Jan 21 2023, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2023, 11:28 AM
RKSimon requested review of this revision.Jan 21 2023, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2023, 11:28 AM
pengfei added inline comments.Jan 22 2023, 6:29 PM
llvm/test/CodeGen/X86/abds-vector-128.ll
159

This seems not correct.
The SDM says When an individual result is too large or too small to be represented in a byte, the result is wrapped around and the low 8 bits are written to the destination element
While LangRef says If the argument is INT_MIN, then the result is also INT_MIN if is_int_min_poison == 0 and poison otherwise.

RKSimon added inline comments.Jan 23 2023, 2:12 AM
llvm/test/CodeGen/X86/abds-vector-128.ll
159

Aren't we making use of the sext to guarantee that the sub result is in bounds: https://alive2.llvm.org/ce/z/5UECxJ

pengfei added inline comments.Jan 23 2023, 3:52 AM
llvm/test/CodeGen/X86/abds-vector-128.ll
159

Oh, yes. My mistake! Sorry for the noise.

dmgreen accepted this revision.Jan 29 2023, 3:38 AM

The DAG combiner changes LGTM.

I would like to get D119075 done in one way or another. It requires creating node for types that are not legal though. I don't think this should prevent that though.

This revision is now accepted and ready to land.Jan 29 2023, 3:38 AM

The DAG combiner changes LGTM.

I would like to get D119075 done in one way or another. It requires creating node for types that are not legal though. I don't think this should prevent that though.

Thanks @dmgreen - I did start looking at ABDS/ABDU legalization but didn't get very far - the ABS(SUB_NSW(X,Y)) pattern in particular is difficult to legalize without instruction bloat - I did wonder about adding NSW flags to the folded ABDS node?

This revision was landed with ongoing or failed builds.Feb 4 2023, 3:26 AM
This revision was automatically updated to reflect the committed changes.
SjoerdMeijer added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10193

We are seeing an AArch64 regression because of this patch, and the isOperationLegalOrCustom -> isOperationLegal change seems to be causing this. We are no longer generating a absolute difference and accumulate instruction, which apparently hinges on this ABDS rewrite first. Looks like all sorts of tests are missing for this, but we will add them.

I don't quite understand this change, why "LegalOrCustom" would not be good enough and how that relates to NSW flags. Any insights would be appreciated. I was also surprised to see:

setOperationAction(ISD::ABDS,             VT, Custom);

in this patch. I.e., the "Custom" surprised me and how that works with the isOperationLegal check.
Again, any tips would be welcome while I look a bit more into this. :)

RKSimon added inline comments.Feb 10 2023, 7:54 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10193

Its a fudge to get around the fact that expansion from ABDS will be at least 3 ops or more - sub(smax(x,y),smin(x,y)) / selectcc(sgt(x,y),sub(x,y),sub(y,x)) / etc. - but if we have an abs instruction then abs(sub_nsw(x,y)) will be better in this case.

A possible (untested) alternative could be:

(TLI.isOperationLegal(ISD::ABDS, VT) || !TLI.isOperationLegalOrCustom(ISD::ABS, VT))
rjj added a subscriber: rjj.Feb 10 2023, 9:22 AM
SjoerdMeijer added inline comments.Feb 14 2023, 6:51 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10193

Thanks, we will be looking into this!

rjj added inline comments.Feb 15 2023, 4:04 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10193

@RKSimon I've tested your suggestion and it still doesn't get us abs(sub nsw) -> abds back in AArch64. I still don't understand the isOperationLegalOrCustom -> isOperationLegal change. Could you please give us a few more pointers please?

RKSimon added inline comments.Feb 15 2023, 4:38 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10193

Well, I did say it was untested.... Short of adding a TLI call (preferABDToABS(EVT)?) I'm not sure what to do here

dmgreen added inline comments.Feb 15 2023, 6:39 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10193

If this is for SVE then we might just be able to add tablegen patterns to cover it, providing they know that it is a nsw sub. It is a bit of a shame to need the extra patterns, so maybe preferABDToABS is better and then it can share the code.

Unfortunately SDAG can be a little awkward when there are multiple backends involved, where "Custom" means too many different things.