Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

ARM32: fix interface dispatch cell address transfer #14090

Merged
merged 1 commit into from
Sep 27, 2017

Conversation

BredPet
Copy link

@BredPet BredPet commented Sep 20, 2017

Signed-off-by: Petr Bred bredpetr@gmail.com

@BredPet
Copy link
Author

BredPet commented Sep 20, 2017

wrong disasm:

movw    r12, #50152     ; 0xc3e8
movt    r12, #64        ; 0x40
ldr.w   r12, [r12]
ldr     r3, [r0, #0]
blx     r12

after fix:

movw    r12, #50136     ; 0xc3d8
movt    r12, #64        ; 0x40
ldr.w   r3, [r12]
ldr     r2, [r0, #0]
blx     r3

After this change we got a working version for ARM Tizen with RyuJIT and ILC/CoreRT compiler!!!
But only for a very simple example ;-)

@BredPet
Copy link
Author

BredPet commented Sep 20, 2017

@Dmitri-Botcharnikov
@sergign60
@alpencolt
@jkotas please review

@@ -3803,7 +3803,11 @@ GenTree* Lowering::LowerVirtualStubCall(GenTreeCall* call)
// on x64 we must materialize the target using specific registers.
addr->gtRegNum = comp->virtualStubParamInfo->GetReg();

// On ARM we must use a proper address in R12(thunk register) without dereferencing.
// So for the jump we use the default register.
#ifndef _TARGET_ARM_
indir->gtRegNum = REG_JUMP_THUNK_PARAM;
Copy link
Member

@jkotas jkotas Sep 20, 2017

Choose a reason for hiding this comment

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

Does the register need to be ever fixed for any platform? Should this rather be deleted?

Copy link
Author

Choose a reason for hiding this comment

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

I'll try it on tests.

@BredPet BredPet force-pushed the interface_dispatch_cell branch from 150a9b4 to 534b508 Compare September 20, 2017 18:38
@BruceForstall
Copy link
Member

What is the requirement here, for CoreRT?

I don't think deleting the line is correct. I thought that x64, at least, required the address in a specific register.

Does the change need to be specific to CoreRT and not change CoreCLR?

If you run diffs for x64, are there any?

@jkotas
Copy link
Member

jkotas commented Sep 21, 2017

I have updated the CoreRT ABI doc in #14120 with CoreRT details.

I do not think any platform requires the target in a specific register. The only requirement is that the indirection that the target was fetched from has to be in a specific register. It is how it is written in https://github.com/dotnet/coreclr/blob/master/Documentation/botr/clr-abi.md at least.

My guess is that REG_JUMP_THUNK_PARAM is a left over from the legacy register allocator that was not able to get the target register scheduled and the target register value had to be hardcoded.

If there is a worry about regressing x64, I think we can keep the change under ifdef - but it may be nice to add a TODO comment that it is probably unnecessary.

@BruceForstall
Copy link
Member

Would it be obvious, if looking at the VM stub code, whether it uses the stub address as currently set up (for amd64, e.g.)?

@jkotas
Copy link
Member

jkotas commented Sep 21, 2017

@BredPet BredPet force-pushed the interface_dispatch_cell branch from 534b508 to 050fb2a Compare September 26, 2017 12:42
@BredPet
Copy link
Author

BredPet commented Sep 26, 2017

@jkotas please review again

@BredPet
Copy link
Author

BredPet commented Sep 26, 2017

@dotnet-bot test Tizen armel Cross Debug Build please

@jkotas
Copy link
Member

jkotas commented Sep 26, 2017

@BruceForstall ?

@BruceForstall
Copy link
Member

@dotnet-bot test Windows_NT arm Cross Checked Build and Test
@dotnet-bot test Windows_NT armlb Cross Checked Build and Test

Signed-off-by: Petr Bred <bredpetr@gmail.com>
@BredPet BredPet force-pushed the interface_dispatch_cell branch from 050fb2a to 74478f4 Compare September 27, 2017 10:07
@BredPet
Copy link
Author

BredPet commented Sep 27, 2017

@dotnet-bot test Windows_NT arm Cross Checked Build and Test please
@dotnet-bot test Windows_NT armlb Cross Checked Build and Test please

@BredPet
Copy link
Author

BredPet commented Sep 27, 2017

@BruceForstall please review again

@BruceForstall BruceForstall merged commit 0d3c684 into dotnet:master Sep 27, 2017
@BredPet BredPet deleted the interface_dispatch_cell branch November 8, 2017 11:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants