Page MenuHomePhabricator

[ISel] Port AArch64 SABD and UABD to DAGCombine
ClosedPublic

Authored by dmgreen on Nov 22 2020, 1:41 PM.

Details

Summary

This ports the AArch64 SABD and USBD over to DAG Combine, where they can be used by more backends (notably MVE in a follow-up patch). The matching code has changed very little, just to handle legal operations and types differently. It selects from (ABS (SUB (EXTEND a), (EXTEND b))), producing a ubds/abdu which is zexted to the original type.

Diff Detail

Event Timeline

dmgreen created this revision.Nov 22 2020, 1:41 PM
dmgreen requested review of this revision.Nov 22 2020, 1:41 PM

What other archs have equivalent instructions?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9095

Rename to VT1 /VT2? I don't think there's anything that technically requires them to be vectors?

llvm/lib/CodeGen/TargetLoweringBase.cpp
816

// Absolute differences

What other archs have equivalent instructions?

@nemanjai Does PPC have equivalent instructions?

dmgreen updated this revision to Diff 307632.Nov 25 2020, 9:10 AM
dmgreen marked an inline comment as done.

Thanks for taking a look.

What other archs have equivalent instructions?

It was MVE I was hoping to share this with, over in the ARM backend. I don't know if any other backend would find this useful too - it would be great if they did. I find it hard enough to know what ARM instructions do some of the time let along other architectures! I believe the X86 instruction (PSADBW) works quite differently, as it includes a reduction step?

Yeah - sum-of-abs-diff is a bit more complex - technically we could use ABD as a stepping stone in pattern matching it

llvm/include/llvm/CodeGen/ISDOpcodes.h
616

Add comments showing both ADDS and ADDU patterns

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9077

naming consistency - combineAbsToAbd or combineABSToABD

dmgreen updated this revision to Diff 307658.Nov 25 2020, 10:55 AM

Updated patterns in comments and renamed to combineABSToABD.

RKSimon added inline comments.Nov 26 2020, 4:19 AM
llvm/include/llvm/Target/TargetSelectionDAG.td
373

Can't these be commutative ?

RKSimon added inline comments.Dec 2 2020, 9:38 AM
llvm/include/llvm/Target/TargetSelectionDAG.td
373
dmgreen added inline comments.Dec 2 2020, 12:31 PM
llvm/include/llvm/Target/TargetSelectionDAG.td
373

Aye, I agree. Sorry I have not gotten back to this yet, and didn't have time over the weekend to update it. I'll try and do so soon.

I did happen to talk to someone how implemented the same thing in gcc and apparently they represent it as max(a, b) - min(a, b). Which I thought was nifty but does show how the whole thing can be commutative.

dmgreen updated this revision to Diff 309547.Dec 4 2020, 8:51 AM

Now with Commutative. Thanks!

RKSimon accepted this revision.Dec 4 2020, 9:09 AM

LGTM

This revision is now accepted and ready to land.Dec 4 2020, 9:09 AM

@dmgreen What happened to this?

I still need to do something with D91921 before it can be used with MVE. I was working on that, but it was a bit slow going and other things had come up in the meantime. I was looking at it recently to try and improve trunc/extend lowering, but need to get some time to sort through that properly. There is also some stuff to do with the way that MVE wants to do lane interleaving that I thought might be an issue, but I'm pretty sure that will be fine.

This part can go in if we need it I think. But the MVE side won't be working well yet, and so there won't be another user other than AArch64.

dmgreen updated this revision to Diff 354689.Jun 26 2021, 11:31 AM

Rebase to current trunk.

This revision was landed with ongoing or failed builds.Jun 26 2021, 11:34 AM
This revision was automatically updated to reflect the committed changes.