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.

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

[ARM] Machine Outliner issue with LDRD/STRD in Thumb mode #49825

Closed
yroux opened this issue May 26, 2021 · 5 comments
Closed

[ARM] Machine Outliner issue with LDRD/STRD in Thumb mode #49825

yroux opened this issue May 26, 2021 · 5 comments
Labels
backend:ARM bugzilla Issues migrated from bugzilla

Comments

@yroux
Copy link
Contributor

yroux commented May 26, 2021

Bugzilla Link 50481
Resolution FIXED
Resolved on Jun 14, 2021 22:51
Version 12.0
OS All
Blocks #48661
CC @luqmana,@nikic,@simonwallis2,@smithp35,@tstellar
Fixed by commit(s) 6c78dbd a95bf58

Extended Description

This issue was reported and described by rust-lang community here:

rust-lang/rust#85351

The problem occurs when the machine outliner extracts a chunck of code which
contains a call and loads or stores a pair of registers from or into the
stack, such as:

bl foo
strd r0, r1, [sp, #​0]
strd r2, r3, [sp, #​8]

LR needs to be saved into the stack when jumping in such an outlined
function and stack's offsets should be changed accordingly, thus the
function should look like:

<OUTLINED_FUNCTION_0>:
str.w lr, [sp, #-8]!
bl foo
strd r0, r1, [sp, #​8]
strd r2, r3, [sp, #​16]
ldr.w lr, [sp], #​8
bx lr

But in the code generated by llvm-12 for Thumb2 targets, the stack offsets
are not patched. In fact, these offsets are patched in the MIR by the
Machine Outliner, but it assumes that immediates encoded in
AddrModeT2_i8s4 instructions are scaled like it is the case for the other
addressing modes.

I'll submit a small patch to fix the outliner which will change the scale
and bit number in that part, but maybe it'd be nice to do a bigger change
to make the encoding more consistent (there are already some fixme notes
about that in the code)

@yroux
Copy link
Contributor Author

yroux commented May 26, 2021

Proposed fix is here:

https://reviews.llvm.org/D103167

@yroux
Copy link
Contributor Author

yroux commented Jun 9, 2021

Fix commited into mainline as: https://reviews.llvm.org/rG6c78dbd4ca1f

@nikic
Copy link
Contributor

nikic commented Jun 9, 2021

Reopening this issue to track LLVM 12 backport. This should be safe to backport, right?

@yroux
Copy link
Contributor Author

yroux commented Jun 9, 2021

Ah yes sure, I think it safe to backport, sorry to have it marked as resolved too quickly ;)

@tstellar
Copy link
Collaborator

Merged: a95bf58

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:ARM bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

3 participants