This is an archive of the discontinued LLVM Phabricator instance.

[DAG] PromoteIntRes_ADDSUBSHLSAT - use promoted ISD::USUBSAT directly
ClosedPublic

Authored by RKSimon on Feb 12 2021, 11:16 AM.

Details

Summary

As discussed on D96413, as long as the promoted bits of the args are zero we can use the basic ISD::USUBSAT pattern directly, without the shifting like we do for other ops.

I think something similar should be possible for ISD::UADDSAT as well, which I'll look at later.

Also, create a ISD::USUBSAT node directly - this will be expanded back by the legalizer later on if necessary.

https://alive2.llvm.org/ce/

Diff Detail

Event Timeline

RKSimon created this revision.Feb 12 2021, 11:16 AM
RKSimon requested review of this revision.Feb 12 2021, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 11:16 AM
Herald added a subscriber: wdng. · View Herald Transcript
nikic accepted this revision.Feb 12 2021, 11:41 AM

LGTM

I think something similar should be possible for ISD::UADDSAT as well, which I'll look at later.

This works for usubsat because the lower bound stays the same (0), while for uaddsat it would change (from 2^bw-1 to 2^new_bw-1). So not sure a similar pattern is possible for that case.

https://alive2.llvm.org/ce/

I think you posted the wrong link there :)

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
816

Personally, I'd drop this assert. The zext is explicitly done up above, and doing a known bits query for this seems weird to me.

This revision is now accepted and ready to land.Feb 12 2021, 11:41 AM
nikic added inline comments.Feb 12 2021, 11:43 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
818

Structurally, it might make sense to move the USUBSAT check before the IsShift || TLI.isOperationLegalOrCustom(Opcode, PromotedType) branch, as we'll now unconditionally take it. In that case you also don't need to change the shift code, and can keep the llvm_unreachable in there (not sure if you switch as written may generate warnings).

craig.topper added inline comments.Feb 12 2021, 12:04 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
818

We could also just dispatch USUBSAT to PromoteIntRes_ZExtIntBinOp in the main switch.

I think something similar should be possible for ISD::UADDSAT as well, which I'll look at later.

This works for usubsat because the lower bound stays the same (0), while for uaddsat it would change (from 2^bw-1 to 2^new_bw-1). So not sure a similar pattern is possible for that case.

What I noticed was that removing UADDSAT from the 'shift' codegen (and always using the default expansion further down) seemed to generate better code - I'll take another look.

We could also just dispatch USUBSAT to PromoteIntRes_ZExtIntBinOp in the main switch.

Thanks - I'd forgotten about that!

This revision was landed with ongoing or failed builds.Feb 13 2021, 4:35 AM
This revision was automatically updated to reflect the committed changes.