This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Rename ADDCARRY/SUBCARRY to UADDO_CARRY/USUBO_CARRY (NFC)
ClosedPublic

Authored by barannikov88 on Apr 12 2023, 11:59 PM.

Details

Summary

This will make them consistent with other overflow-aware nodes.

Diff Detail

Event Timeline

barannikov88 created this revision.Apr 12 2023, 11:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2023, 11:59 PM
barannikov88 published this revision for review.Apr 13 2023, 12:35 AM
barannikov88 edited the summary of this revision. (Show Details)
barannikov88 added reviewers: RKSimon, craig.topper.
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 12:36 AM

Can you put the new names in the description? That way once it's commited the new names will be in the commit message.

barannikov88 retitled this revision from [SelectionDAG] Rename ADDCARRY/SUBCARRY for consistency with other nodes to [SelectionDAG] Rename ADDCARRY/SUBCARRY to UADDO_CARRY/USUBO_CARRY.Apr 13 2023, 12:49 AM

Can you put the new names in the description? That way once it's commited the new names will be in the commit message.

Updated the title

ADDCARRY/SUBCARRY have been the only overflow-aware nodes for a long time.

UADDO and USUBO are much older than ADDCARRY/SUBCARRY.

llvm/include/llvm/CodeGen/ISDOpcodes.h
301

I think too many things got renamed here. The comparison to [US]{ADD,SUB}O matches the rest of the description.

ADDCARRY/SUBCARRY have been the only overflow-aware nodes for a long time.

UADDO and USUBO are much older than ADDCARRY/SUBCARRY.

Thanks, I'll update the description. Do you maybe know the true reason for the discrepancy?

llvm/include/llvm/CodeGen/ISDOpcodes.h
301

The comparison with [US]{ADD,SUB}O is inaccurate.
The U version of these produces a carry (same as the former ADD/SUBCARRY),
the S version produces an overflow. The comment says they both produce an overflow.
I think the intention was to compare them with S{ADD,SUB}O_CARRY.

I'll revert this part so that this patch does not change the semantics of the comments.

RKSimon added inline comments.Apr 13 2023, 1:36 AM
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
3997

rename?

4003

rename?

barannikov88 edited the summary of this revision. (Show Details)

Restore the semantics of a comment

barannikov88 marked an inline comment as done.Apr 13 2023, 1:56 AM
barannikov88 added inline comments.
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
4003

Might make sense, but I'm not sure if the names of these two functions are derived from node names.

barannikov88 edited the summary of this revision. (Show Details)Apr 13 2023, 1:57 AM
barannikov88 retitled this revision from [SelectionDAG] Rename ADDCARRY/SUBCARRY to UADDO_CARRY/USUBO_CARRY to [SelectionDAG] Rename ADDCARRY/SUBCARRY to UADDO_CARRY/USUBO_CARRY (NFC).Apr 13 2023, 4:08 PM

SGTM, do you want to get the input from the target specialists that you update the most (ARM/AMDGPU)?

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
4003

Only a minor suggestion - it shouldn't derail this patch.

RKSimon accepted this revision.Apr 25 2023, 12:32 AM

LGTM - cheers

This revision is now accepted and ready to land.Apr 25 2023, 12:32 AM

Thanks for the reviews.

This revision was landed with ongoing or failed builds.Apr 29 2023, 12:00 PM
This revision was automatically updated to reflect the committed changes.