Page MenuHomePhabricator

[LoongArch] Add GHC Calling Convention
ClosedPublic

Authored by lrzlin on Nov 5 2022, 11:39 AM.

Diff Detail

Event Timeline

lrzlin created this revision.Nov 5 2022, 11:39 AM
lrzlin requested review of this revision.Nov 5 2022, 11:39 AM

Is GHC buildable now? If not, such changes probably should be deferred until verification can actually be made.

llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
1636

delete blank line

Is GHC buildable now? If not, such changes probably should be deferred until verification can actually be made.

Yes, with some modifications, both GHC HEAD and 9.4.2 Release could pass build and testsuite. However, GHC haven't implemented the support of LLVM 14 and higher versions yet, this may lead to a tardive merge in GHC side.

Thanks. But I have to admit I know little about GHC. Maybe I have to read some docs before reviewing the change.

llvm/test/CodeGen/LoongArch/ghc-cc.ll
22

fs4?

lrzlin updated this revision to Diff 473491.Nov 6 2022, 1:43 AM

Fix typos of the register mapping and delete empty lines.

@lrzlin

I don't quite understand what you mean by` CodeModel=Small` not implemented? The default CodeModel we currently define is Small, which is the same as gcc's mcmodel=normal. Medium only uses pcalau12i+jirl to increase the jumping capability relative to pc when the function is called, and the others remain unchanged, which is also the same as gcc's mcmodel=medium, currently we only provide these two codemodels(D137394 for Medium), gcc also has a mcmodel=extreme(support 64bit addressing capability). For this(extreme), I haven't figured out which CodeModel corresponds to llvm, maybe large will be better. It's going to take a while to support it, because currently no one uses it except jit. The loongson gcc team may(just maybe) also consider the implementation of mcmodel=large (support 52bit addressing capability). so? large = large? or large=extreme? - -|||

lrzlin added a comment.Nov 6 2022, 9:37 PM

@wangleiat
I have consulted the RISC-V codemodel here, which CodeModel::Small is represented by addi (lui %hi(sym)) %lo(sym), using for accessing the first 2GiB of address space, I think it could be implemented in LoongArch also, or LoongArch have a different codemodel here?

@wangleiat
I have consulted the RISC-V codemodel here, which CodeModel::Small is represented by addi (lui %hi(sym)) %lo(sym), using for accessing the first 2GiB of address space, I think it could be implemented in LoongArch also, or LoongArch have a different codemodel here?

RISC-V only distinguishes when relocation-model=static is specified, but it is not required on LoongArch. On LoongArch, regardless of relocation-model=everything, it is sufficient to use pc-relative instructions.

@wangleiat
I have consulted the RISC-V codemodel here, which CodeModel::Small is represented by addi (lui %hi(sym)) %lo(sym), using for accessing the first 2GiB of address space, I think it could be implemented in LoongArch also, or LoongArch have a different codemodel here?

RISC-V only distinguishes when relocation-model=static is specified, but it is not required on LoongArch. On LoongArch, regardless of relocation-model=everything, it is sufficient to use pc-relative instructions.

Without the isPositionIndepent() and code-model check, original getAddr() will map the PseudoLA_GOT pattern in GHC calling convention (in ghc-cc.ll), which is obviously incorrect (should be LA_PCREL). In case that all relocation-models are sufficient to use pc-relative instructions, both CodeModel=Small and CodeModel=Medium could keep being LA_PCREL.

lrzlin edited the summary of this revision. (Show Details)Nov 7 2022, 12:43 AM

@wangleiat
I have consulted the RISC-V codemodel here, which CodeModel::Small is represented by addi (lui %hi(sym)) %lo(sym), using for accessing the first 2GiB of address space, I think it could be implemented in LoongArch also, or LoongArch have a different codemodel here?

RISC-V only distinguishes when relocation-model=static is specified, but it is not required on LoongArch. On LoongArch, regardless of relocation-model=everything, it is sufficient to use pc-relative instructions.

Without the isPositionIndepent() and code-model check, original getAddr() will map the PseudoLA_GOT pattern in GHC calling convention (in ghc-cc.ll), which is obviously incorrect (should be LA_PCREL). In case that all relocation-models are sufficient to use pc-relative instructions, both CodeModel=Small and CodeModel=Medium could keep being LA_PCREL.

Personally I think using GOT or PCREL should have nothing to do with isPositionIndependent . If this test case uses -relocation-model=pic on RISC-V, it should also generate got related instructions.
In addition, I really don't understand the calling convention of ghc, is it not allowed to use got with ghc?

lrzlin added a comment.Nov 7 2022, 2:16 AM

@wangleiat
Basically, what GHC calling convention do is mapping the virtual registers of STG Machine (part of the Haskell Runtime System) into real registers, so here in ghc-cc.ll must be pcrel, in short, it isn't a function call, but a simple value load process, using got here will lead to load a symbol twice, which isn't a function symbol, so it will not present in GOT table. More information about this calling convention could find on GHC llvm porting
Also, using -pic or not is determined by GHC itself, and judge by its knowing behavior, isPositionIndependent is needed to avoid generating got patterns while compiling it using llvm backend.

xen0n added a comment.Dec 6 2022, 10:24 PM

Thanks for the contribution, I've checked against your GHC-side changes and the register convention looks good. Apart from some minor nits this should be good to go.

llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
445–446

Is the comment still necessary? The small codemodel case is being implemented right below...

llvm/test/CodeGen/LoongArch/ghc-cc.ll
3

In LoongArch testcases --mattr (double-dash) is used throughout.

85–101

Please convert to opaque pointers.

lrzlin updated this revision to Diff 480817.Dec 7 2022, 2:18 AM

Following @xen0n's advice: Fix --mattr and replace pointer types with opaque pointers in ghc-cc.ll, delete TODO in CodeModel:Small and make it fall through to CodeModel:Medium.

xen0n accepted this revision.Dec 13 2022, 6:17 PM

LGTM, thanks very much for the contribution!

As discussed elsewhere, there's the possibility of making use of $s9 because we don't need a frame pointer, but as the GHC changes are already merged, this can come later with another pair of coordinated changes.

Any opinions on this from other reviewers?

This revision is now accepted and ready to land.Dec 13 2022, 6:17 PM
  1. Could you please rebase the change? I failed to apply it to my codebase.
  2. It's better to add -U999999 when generating the change so that we can have more context. See: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
lrzlin updated this revision to Diff 482741.Dec 14 2022, 1:10 AM
lrzlin marked 3 inline comments as done.
  1. Could you please rebase the change? I failed to apply it to my codebase.
  2. It's better to add -U999999 when generating the change so that we can have more context. See: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Rebased and add -U999999 while generating the patch.

wangleiat added inline comments.Dec 19 2022, 11:25 PM
llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
474

I think there is no need to judge here. The following modifications(switch codemodel) should also be unnecessary. This modification will change the semantics of dso_preemptable. https://llvm.org/docs/LangRef.html#runtime-preemption-specifiers

llvm/test/CodeGen/LoongArch/ghc-cc.ll
7–26

dso_local can be used.

lrzlin updated this revision to Diff 485187.Dec 24 2022, 1:15 AM

Using dso_local in ghc-cc.ll and revert the changes in getAddr() to keep the semantics of dso_preemtable.

wangleiat accepted this revision.Dec 24 2022, 1:21 AM

LGTM, Thanks.

xen0n edited the summary of this revision. (Show Details)Dec 25 2022, 8:00 AM
xen0n accepted this revision.Dec 25 2022, 8:02 AM

This is cleaner than previous revisions (which contained code model related changes) which is good. I've slightly adjusted the patch summary to remove the trivial bits and now inaccurate descriptions.

Thanks very much for the contribution!

SixWeining added inline comments.Dec 25 2022, 5:52 PM
llvm/lib/Target/LoongArch/LoongArchFrameLowering.cpp
194–197

Please remove useless spaces.

199

Ditto.

328–331

Ditto.

333

Ditto.

llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
2539–2540

Ditto.

2542

Ditto.

llvm/test/CodeGen/LoongArch/ghc-cc.ll
108

Ditto.

lrzlin updated this revision to Diff 485277.Dec 26 2022, 2:36 AM
lrzlin marked 2 inline comments as done.
lrzlin marked 7 inline comments as done.

Delete blank lines as @SixWeining suggest.

SixWeining accepted this revision.Dec 26 2022, 2:40 AM

Delete blank lines as @SixWeining suggest.

Thanks. Do you have commit access? If not, I could commit it for you if you leave the name and email here.

Delete blank lines as @SixWeining suggest.

Thanks. Do you have commit access? If not, I could commit it for you if you leave the name and email here.

I do not have commit access, my name is Lin Runze and my email is lrzlin@163.com, thanks for your help.

This revision was landed with ongoing or failed builds.Dec 26 2022, 3:37 AM
This revision was automatically updated to reflect the committed changes.

Unfortunately it breads the build of buildbot: https://lab.llvm.org/staging/#/builders/236/builds/206. Please fix it as soon as possible, otherewise it would be reverted.

Unfortunately it breads the build of buildbot: https://lab.llvm.org/staging/#/builders/236/builds/206. Please fix it as soon as possible, otherewise it would be reverted.

D140670 fixs it.

lrzlin updated this revision to Diff 485308.Dec 26 2022, 8:34 AM