This is an archive of the discontinued LLVM Phabricator instance.

MachineIRBuilder: Rename buildMerge. NFC
ClosedPublic

Authored by rovka on Jan 10 2023, 4:24 AM.

Details

Reviewers
paquette
aemerson
foad
Group Reviewers
Restricted Project
Commits
rGf95a5fbe7ce1: MachineIRBuilder: Rename buildMerge. NFC
Summary

buildMerge may build a G_MERGE_VALUES, G_BUILD_VECTOR or
G_CONCAT_VECTORS. Rename it to buildMergeLikeOp.

This is a follow-up suggested in https://reviews.llvm.org/D140964

Diff Detail

Event Timeline

rovka created this revision.Jan 10 2023, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2023, 4:24 AM
rovka requested review of this revision.Jan 10 2023, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2023, 4:24 AM
rovka added reviewers: Restricted Project, paquette, aemerson.Jan 10 2023, 4:28 AM
foad accepted this revision.Jan 10 2023, 4:32 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jan 10 2023, 4:32 AM
rovka added a comment.Jan 10 2023, 4:37 AM

Thanks for the quick review! I'm going to leave this around until tomorrow, in case anyone wants to bikeshed about the new name.

aemerson accepted this revision.Jan 10 2023, 10:04 AM

Seems perfectly reasonable to me.

Tiny driveby bikeshed (feel free to ignore): buildMergeLikeInstr? It is longer, but Op seems to more commonly mean "Operand" here (save for some stuff like buildAssertOp).

Tiny driveby bikeshed (feel free to ignore): buildMergeLikeInstr? It is longer, but Op seems to more commonly mean "Operand" here (save for some stuff like buildAssertOp).

Sounds good, thanks :)

foad added a comment.Jan 11 2023, 1:05 AM

Tiny driveby bikeshed (feel free to ignore): buildMergeLikeInstr? It is longer, but Op seems to more commonly mean "Operand" here (save for some stuff like buildAssertOp).

I'm not a huge fan of the "Op" part either, but there is a precedent for it in the GMergeLikeOp wrapper class.

rovka added a comment.Jan 11 2023, 1:41 AM

Tiny driveby bikeshed (feel free to ignore): buildMergeLikeInstr? It is longer, but Op seems to more commonly mean "Operand" here (save for some stuff like buildAssertOp).

I'm not a huge fan of the "Op" part either, but there is a precedent for it in the GMergeLikeOp wrapper class.

There could be an argument made that both GMergeLikeOp and buildAssertOp should use Instr instead of Op, too... Should I just rename all of them?

foad added a comment.Jan 11 2023, 1:58 AM

Tiny driveby bikeshed (feel free to ignore): buildMergeLikeInstr? It is longer, but Op seems to more commonly mean "Operand" here (save for some stuff like buildAssertOp).

I'm not a huge fan of the "Op" part either, but there is a precedent for it in the GMergeLikeOp wrapper class.

There could be an argument made that both GMergeLikeOp and buildAssertOp should use Instr instead of Op, too... Should I just rename all of them?

I don't mind either way.

Tiny driveby bikeshed (feel free to ignore): buildMergeLikeInstr? It is longer, but Op seems to more commonly mean "Operand" here (save for some stuff like buildAssertOp).

I'm not a huge fan of the "Op" part either, but there is a precedent for it in the GMergeLikeOp wrapper class.

There could be an argument made that both GMergeLikeOp and buildAssertOp should use Instr instead of Op, too... Should I just rename all of them?

As a separate commit yes that seems fine to me.

Tiny driveby bikeshed (feel free to ignore): buildMergeLikeInstr? It is longer, but Op seems to more commonly mean "Operand" here (save for some stuff like buildAssertOp).

I'm not a huge fan of the "Op" part either, but there is a precedent for it in the GMergeLikeOp wrapper class.

There could be an argument made that both GMergeLikeOp and buildAssertOp should use Instr instead of Op, too... Should I just rename all of them?

As a separate commit yes that seems fine to me.

+1 to renaming in a pre-patch

This revision was automatically updated to reflect the committed changes.