Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ARM and AArch64 and Win64 ABI #18689

Merged
merged 2 commits into from
Sep 27, 2016
Merged

Fix ARM and AArch64 and Win64 ABI #18689

merged 2 commits into from
Sep 27, 2016

Conversation

yuyichao
Copy link
Contributor

@yuyichao yuyichao commented Sep 27, 2016 •
edited
Loading

The AArch64 (and likely ARM) requires certain arguments to be effectively casting through memory to a larger size before passing as argument or returning. From AAPCS64

B.4 If the argument type is a Composite Type then the size of the argument is rounded up to the nearest multiple of 8 bytes.

and

C.10 <......> The argument is passed as though it had been loaded into the registers from a double-word-aligned address with an appropriate sequence of LDR instructions loading consecutive registers from memory (the contents of any unused parts of the registers are unspecified by this standard).

These are modeled by LLVM as an array type of the the element type being a register size integer (i64 on AArch64, i32 on ARM) so the cast through memory should handle this case.

The commit also added an assertion to make sure we are not storing an object larger then the GC allocation size and fix the store alignment for small objects that are not 16bytes aligned on 32bit.

The argument passing is not changed since we are always storing a smaller value to a larger slot which should be fine.

Add a few ccall test for the related cases.
Also test both mutable and immutable types since they follows different code path in codegen. (The issue would have trigger a ccall test failure if immutable types were used in the tests).

I noticed the issue since it triggers a failure in the recently added VecElement test in core

@yuyichao yuyichao added system:arm ARMv7 and AArch64 compiler:codegen Generation of LLVM IR and native code backport pending 0.5 labels Sep 27, 2016
@yuyichao
Copy link
Contributor Author

Tentatively added backport pending label since this fixes a bug. The change isn't entirely trivial though.

vchuravy
vchuravy reviewed Sep 27, 2016
View reviewed changes
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you have a chance to test this on PPC? Otherwise I can run the tests.

src/ccall.cpp
#if JL_LLVM_VERSION >= 30600
jl_ExecutionEngine->getDataLayout();
#else
*jl_ExecutionEngine->getDataLayout();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation?

src/ccall.cpp
#if JL_LLVM_VERSION >= 30600
jl_ExecutionEngine->getDataLayout();
#else
*jl_ExecutionEngine->getDataLayout();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation?

src/julia_internal.h
(void)sz;
return 16;
#elif defined(_CPU_ARM_) || defined(_CPU_PPC_)
return rtsz < 4 : 8 : 16;
Copy link
Member

@vchuravy vchuravy Sep 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be (rtsz < 4) ? 8 : 16; I have never seen the syntax cond : x : y;

src/julia_internal.h
return 8;
// szclass 12
if (sz < 8)
return 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment why this is 4 might be helpful. It seems counter-intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is

// szclass 12

@yuyichao
Copy link
Contributor Author

Did you have a chance to test this on PPC

No, I don't have hardware. I also haven't tested on ARM yet but will do soon.

@tkelman
Copy link
Contributor

tkelman commented Sep 27, 2016

win64 appveyor failure is in ccall, looks related

@yuyichao
Fix ARM and AArch64 ABI
551cd52 
Also add related ccall test and test both mutable and immutable types.
@yuyichao
Copy link
Contributor Author

I'm wondering if it's because of the change in codegen or tests.

@yuyichao
Copy link
Contributor Author

Looks like the new test triggers a bug on win64 ABI. It'll probably take some time before I can debug this since my windows VM is not working ATM.

@yuyichao
Copy link
Contributor Author

For Struct8 the ccall signature we generate is i24(i24, i8). I'm not sure how the win64 abi works in llvm but it's very likely not this.....

@vtjnash
Copy link
Member

vtjnash commented Sep 27, 2016

That test is exercising the Win64 ABI issues mentioned at #14215 (comment)

@yuyichao
Copy link
Contributor Author

.... ok, that's why we don't have a test for this.... I added the test because this is the unambiguous case that we need a memory cast between different sizes on both ARM and AArch64.

@yuyichao
Copy link
Contributor Author

Win64 ccall tests should pass now according to local test.

@yuyichao yuyichao changed the title Fix ARM and AArch64 ABI Fix ARM and AArch64 and Win64 ABI Sep 27, 2016
@yuyichao
Copy link
Contributor Author

Local ARM tests also passed.

@vtjnash
Copy link
Member

vtjnash commented Sep 27, 2016

lgtm. we still need to fix the win32 and x64 ABIs for the issues mentioned in the comment linked above, and elsewhere. but it this passes, there's no reason to hold this up.

@vchuravy
Copy link
Member

local PPC tests are hanging. Give me a bit of time to see if this is the cause

@vchuravy vchuravy merged commit bfb1f79 into master Sep 27, 2016
@tkelman tkelman deleted the yyc/codegen/aarch64-abi branch September 27, 2016 22:25
tkelman pushed a commit that referenced this pull request Feb 22, 2017
@yuyichao @tkelman
Fix ARM and AArch64 ABI
c330c12 
Also add related ccall test and test both mutable and immutable types.

(cherry picked from commit 551cd52)
ref #18689
tkelman pushed a commit that referenced this pull request Feb 22, 2017
@yuyichao @tkelman
Fix win64 ABI for non-power-of-two size structs
731ed27 
(cherry picked from commit dd603f0)
ref #18689
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vchuravy vchuravy vchuravy left review comments

Assignees
No one assigned
Labels
compiler:codegen Generation of LLVM IR and native code system:arm ARMv7 and AArch64
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yuyichao @tkelman @vtjnash @vchuravy