This is an archive of the discontinued LLVM Phabricator instance.

Revert "HHVM calling conventions."
ClosedPublic

Authored by Amir on Apr 23 2022, 9:20 AM.

Details

Summary

This reverts commit cce239c45d6ef3865a017b5b3f935964e0348734.

HHVM calling conventions are unused. Remove them by partially reverting the commit.

Diff Detail

Event Timeline

Amir created this revision.Apr 23 2022, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2022, 9:20 AM
Amir requested review of this revision.Apr 23 2022, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2022, 9:20 AM
Amir edited the summary of this revision. (Show Details)Apr 23 2022, 9:27 AM
Amir updated this revision to Diff 425051.Apr 25 2022, 4:04 PM

Addressed CodeGen/X86/statepoint-fastregalloc.mir test failure

Amir updated this revision to Diff 495649.Feb 7 2023, 2:56 PM

Rebase

Amir added a reviewer: MaskRay.Feb 7 2023, 2:56 PM
MatzeB added a comment.Feb 7 2023, 3:32 PM

Makes sense to me to drop this and I like the simplification.

There is a chance that an out-of-tree target is overriding the getStackAlignmentSkew callback, but given that is at least pretty unlikely it's probably good to try to remove first and possibly bringing it back if there really was a user...

Added one nitpick but otherwise seems good to go.

llvm/include/llvm/IR/CallingConv.h
161–164

Maybe rename to make it more explicit this is gone now?

MatzeB added inline comments.Feb 7 2023, 3:34 PM
llvm/test/CodeGen/X86/statepoint-fastregalloc.mir
24

I guess this comment needs to be adapted or removed?

Amir updated this revision to Diff 495657.Feb 7 2023, 3:35 PM

Rename CallingConv enum items

Amir marked an inline comment as done.Feb 7 2023, 3:38 PM
Amir added inline comments.
llvm/test/CodeGen/X86/statepoint-fastregalloc.mir
24

If I only replace the register name, would it still make sense? I'm removing csr_64_hhvm and apparently it has an effect on statepoint operands.

maksfb added a comment.Feb 7 2023, 3:39 PM

For all I know, HHVM is not using it. Looks good on my end. Thanks.

Amir updated this revision to Diff 495661.Feb 7 2023, 3:40 PM

Rename the register in statepoint-fastregalloc.mir test

Amir marked an inline comment as done.Feb 7 2023, 3:41 PM
MatzeB added inline comments.Feb 7 2023, 3:46 PM
llvm/test/CodeGen/X86/statepoint-fastregalloc.mir
24

This feels like they just choose the HHVM convention to show the concept. This feels like they just choose it because HHVM convention only had a single callee-saved register to choose from to make the test more stable. But at a first glance feels fine to switch to any other callee saved reg of the default calling convention instead...
You should subscribe the original author(s) of the test to this diff in case they have a comment.

Amir added a subscriber: dantrushin.Feb 7 2023, 3:48 PM
Amir added inline comments.
llvm/test/CodeGen/X86/statepoint-fastregalloc.mir
24

Thanks!

@dantrushin – does this change look good to you?

MatzeB accepted this revision.Feb 7 2023, 3:58 PM

LGTM

This revision is now accepted and ready to land.Feb 7 2023, 3:58 PM
MatzeB added a comment.Feb 7 2023, 3:59 PM

Please note build failures in the CI and fix before landing.

Amir updated this revision to Diff 495672.Feb 7 2023, 4:01 PM

Rename calling convention enum members elsewhere

MaskRay accepted this revision.Feb 7 2023, 4:19 PM
Amir updated this revision to Diff 495696.Feb 7 2023, 5:41 PM

Update mlir LLVMEnums

dantrushin added inline comments.Feb 8 2023, 6:47 AM
llvm/test/CodeGen/X86/statepoint-fastregalloc.mir
24

Could you please replace csr_hhvm with csr_64_cxx_tls_darwin_pe instead? And register will change to $rbp.

Amir updated this revision to Diff 495919.Feb 8 2023, 12:31 PM

Update test

Amir marked 2 inline comments as done.Feb 8 2023, 12:31 PM
This revision was automatically updated to reflect the committed changes.
llvm/lib/Target/X86/X86RegisterInfo.cpp