Page MenuHomePhabricator

AArch64 : Implement GHC calling convention.
ClosedPublic

Authored by erikd on Jan 7 2015, 10:55 PM.

Diff Detail

Event Timeline

erikd updated this revision to Diff 17885.Jan 7 2015, 10:55 PM
erikd retitled this revision from to AArch64 : Implement GHC calling convention..
erikd updated this object.
erikd edited the test plan for this revision. (Show Details)
erikd added reviewers: t.p.northover, garious.
erikd added a comment.EditedJan 7 2015, 11:00 PM

This patch was previously discussed here: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20141103/243180.html .

WIth this patch applied to llvm-3.5 I can cross-compile simple haskell programs (haven't tried anything complicated) from x86_64-linux to aarch64-linux and generate binaries that execute correctly.

I can't use llvm-3.6 or git HEAD because the current GHC LLVM backend doesn't support those versions yet. When GHC is updated it is extremely unlikely that any changes would be required for this AArch64 GHC calling convention code.

If possible, I'd like to see this patch land in the llvm 3.6 release.

erikd updated this revision to Diff 17886.Jan 7 2015, 11:24 PM

Fix author email address.

erikd added a subscriber: Unknown Object (MLST).Jan 8 2015, 11:51 AM
garious edited edge metadata.Jan 9 2015, 11:58 AM

Erik, thanks for doing this work. I'd love to see GHC AArch64 support make it into 3.6. I'd be more comfortable with this patch if there was a bit more information about the .td file changes and if the test had coverage similar to the X86_64 one (see: test/CodeGen/X86/ghc-cc64.ll).

lib/Target/AArch64/AArch64CallingConvention.td
208

Is there a document you can reference here?

213

I'm having trouble groking this. It looks like a combination of the X86 GHC version and CC_AArch64_AAPCS (perhaps an old version?). I wonder, is it possible to inherit from CC_AArch64_AAPCS and tweak the derived version for GHC?

test/CodeGen/AArch64/ghc-tcreturn-lowered.ll
14

Does this case add coverage? Compared to the one above, seems like it would only affect the bitcode just before the tail call.

erikd updated this revision to Diff 18071.Jan 12 2015, 9:46 PM
erikd edited edge metadata.
  • Added comments documenting the GHC calling convention with references to GHC Wiki and the GHC source code.
  • Significant improvements to the ghccc tests (borrowing heavily from the x86-64 ghccc test).

Thanks Erik. Good test and looks consistent with the X86_64 implementation. I'll try to find one more person to review. Hopefully we can land this one soon.

garious set the repository for this revision to rL LLVM.

Ping? I know its only been up a week or so, but I was hoping to get this into the 3.6 release.

The chances of this commit breaking anything other the the GHC calling convention on AArch64 is rather small and without this patch, the GHC calling convention for AArch64 doesn't work at all.

rengolin edited edge metadata.Jan 14 2015, 1:51 AM

Hi Erik,

The changes look harmless to me, but I'd like to see slightly better tests before this goes in.

cheers,
--renato

test/CodeGen/AArch64/ghc-cc.ll
28 ↗(On Diff #18071)

If you're checking for the absence of prologue, I'd recommend you used CHECK-NOT, instead of CHECK-NEXT with some instruction, because:

  1. It tells us what the test is about, and
  2. It's less fragile to codegen changes.
46 ↗(On Diff #18071)

The more CHECK-NEXT you use, the more fragile the test will be to codegen changes. Though, in these cases, CHECK-DAG will not be as efficient/correct.

Maybe what you should do, since there are no prologues and no stack (I'm assuming), you should create multiple functions, one for each type of parameter, and test it with CHECK-NEXT in an atomic way (for each example, a pair of CHECK + CHECK-NEXT *after* the CHECK-LABEL).

erikd updated this revision to Diff 18373.Jan 19 2015, 1:50 AM
erikd edited edge metadata.
  • Simplify tests to reduce chance of false failure.

Apart from the typo, LGTM. Please, commit with the change.

cheers,
--renato

test/CodeGen/AArch64/ghc-cc.ll
38 ↗(On Diff #18373)

typo :)

erikd updated this revision to Diff 18375.Jan 19 2015, 2:41 AM
  • Fix typoes.
rengolin accepted this revision.Jan 19 2015, 2:59 AM
rengolin edited edge metadata.
rengolin added inline comments.
test/CodeGen/AArch64/ghc-cc.ll
37 ↗(On Diff #18375)

Would be good to have a CHECK-LABEL here, too.

This revision is now accepted and ready to land.Jan 19 2015, 2:59 AM
garious closed this revision.Jan 19 2015, 9:43 AM

Committed in r226473. Renato, I added the CHECK-LABEL you mentioned.

Thanks Erik!