Page MenuHomePhabricator

[AArch64] Add custom lowering for ISD::ABS
ClosedPublic

Authored by craig.topper on Nov 25 2020, 10:43 PM.

Details

Summary

Instead of trying to pattern match the code produced by ISD::ABS expansion, just custom legalize ISD::ABS to the desired sequence.

The one test change is because a DAG combine for (neg (abs)) is no longer firing because ISD::ABS is now Custom instead of Expand.

Diff Detail

Event Timeline

craig.topper created this revision.Nov 25 2020, 10:43 PM
craig.topper requested review of this revision.Nov 25 2020, 10:43 PM

@craig.topper Is the plan to add ISD::NABS at some point?

Can you give more context as to why this is desirable? Is this to do with canonicalizing towards abs intrinsics? Or adding different lowering?

I am considering adding NABS. My original thought was that the DAG combine for NABS was unnecessary on some targets if we expanded to srai+xor+sub instead of srai+add+xor. The the neg and the sub would be free to combine on their own. Of course that doesnt work for targets with custom sequences but the custom sequences probably have better NABS forms anyway. Like csneg with an inverse condition. Or smin(neg)) on other targets. This is where a NABS node could be useful.

Aarch64 is matching the srai+add+xor sequence so it broke when I tried changing it. It looks like this combine predates the existence of ISD::ABS. Using Custom lowering seems like a more standard way to handle it now.

OK thanks. Custom lowering via abs is probably equal to or better than a dag combine as far as I understand, considering we should have enough combines for ABS.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3981

Can we just generate a SUBS (which becomes the CMP) and a CSNEG directly? That should avoid needing to create the two sub's I think.

craig.topper added inline comments.Nov 28 2020, 2:26 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3981

Maybe. I wasn't clear why AArch64 has both AArch64ISD::CSNEG and isel patterns to form CSNEG from CSEL+neg.

LGTM @dmgreen any more comments?

dmgreen accepted this revision.Dec 2 2020, 12:27 PM

Sorry. I read that as "let me look into if we can simplify this".

If you want to do it like this then that sounds fine to me, as far as I understand. LGTM.

This revision is now accepted and ready to land.Dec 2 2020, 12:27 PM
This revision was automatically updated to reflect the committed changes.