This is an archive of the discontinued LLVM Phabricator instance.

[X86] Update RET/LRET instruction to use the same naming convention as IRET (PR36876). NFC
ClosedPublic

Authored by RKSimon on Nov 5 2021, 11:37 AM.

Details

Summary

Be more consistent in the naming convention for the various RET instructions to help prevent future scheduler model mismatches like those that were only addressed in D44687.

Diff Detail

Event Timeline

RKSimon created this revision.Nov 5 2021, 11:37 AM
RKSimon requested review of this revision.Nov 5 2021, 11:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2021, 11:37 AM

Not sure if I understand it correct or not. I used to take the number as system mode. E.g., CALL16m, POP32r etc. And W/L/Q as the size of memory operand. Maybe we should change RETL etc. to RET32 instead?

RKSimon planned changes to this revision.Nov 6 2021, 1:21 AM

Not sure if I understand it correct or not. I used to take the number as system mode. E.g., CALL16m, POP32r etc. And W/L/Q as the size of memory operand. Maybe we should change RETL etc. to RET32 instead?

Sure, that makes sense as well

RKSimon updated this revision to Diff 385266.Nov 6 2021, 5:50 AM
RKSimon retitled this revision from [X86] Update IRET instruction to use the same naming convention as RET/LRET (PR36876). NFC to [X86] Update RET/LRET instruction to use the same naming convention as IRET (PR36876). NFC.
RKSimon updated this revision to Diff 385268.Nov 6 2021, 6:17 AM

update tests

RKSimon planned changes to this revision.Nov 6 2021, 6:30 AM

more tests needs updating...

RKSimon updated this revision to Diff 385335.Nov 7 2021, 4:12 AM

update tests

pengfei accepted this revision.Nov 7 2021, 5:40 AM

Wow, I haven't thought we have so many use of the MI opcodes. Maybe I shouldn't have suggested this way :)

This revision is now accepted and ready to land.Nov 7 2021, 5:40 AM

Wow, I haven't thought we have so many use of the MI opcodes. Maybe I shouldn't have suggested this way :)

Cheers - I agree - I didn't expect the patch to be so bulky, but still I think it makes sense to get this done once and for all!

This revision was landed with ongoing or failed builds.Nov 7 2021, 7:09 AM
This revision was automatically updated to reflect the committed changes.