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

i#3823 multi-phase drreg: Delay slot id label #4925

Merged
merged 10 commits into from
May 27, 2021

Conversation

abhinav92003
Copy link
Contributor

@abhinav92003 abhinav92003 commented May 27, 2021

Moves label that contains slot id for register spill/restore instrs to after the instr instead of
before. The free spill slot selection logic that makes use of these labels scans instrs after
the given one, so we may miss the label if it is placed before.

Fixes order of app val spill and tool val restore instrs after an instr that reads and writes a
spilled reg. This was to take into account the label which is now after the tool restore instr.

Adds test to verify restoration of reg that was reserved in multiple phases on a fault, for
X86 and AARCHXX.

Also adds AARCHXX variant of the multi-phase slot conflict test, and extends it to also
check proper restoration of app val (under normal operation, as opposed to under a
fault which is done by the above test). The existing test only verified whether the slot
used in different phases was different.

Adds a note to the label instrs added by drreg-test to mark instrumentation locations. This
is to avoid conflicts with other label instrs.

Issue: #3823, #2985

Moves label that contains slot id for register spill/restore instrs to after the instr instead of
before. The free spill slot selection logic that makes use of these labels scans instrs *after*
the given one, so we may miss the label if it is placed before.

Fixes order of app val spill and tool val restore instrs after an instr that reads and writes a
spilled reg. This was to take into account the label which is now after the tool restore instr.

Adds test to verify restoration of reg that was reserved in multiple phases on a fault, for
X86 and AARCHXX.

Also adds AARCHXX variant of the multi-phase slot conflict test.

Adds a note to the label instrs added by drreg-test to mark instrumentation locations. This is
to avoid conflicts with other lqbel instrs.

Issue: #3823, #2985
We do not want any fault thrown by test_asm to be caught by any of our signals handlers, and possibly confuse them.
Also use udf instr instead of jump to invalid pc (0) to fail a test.  This was supposed to be part of previous commit, but wasn't due to error.

Formatting fixes
@abhinav92003 abhinav92003 marked this pull request as ready for review May 27, 2021 07:07
ext/drreg/drreg.c Show resolved Hide resolved
@abhinav92003
Copy link
Contributor Author

Seems that drreg-test isn't one of the tests we run on ARM QEMU:

set_tests_properties(

I suppose there was some issue? I can try again.

@abhinav92003
Copy link
Contributor Author

Seems that drreg-test isn't one of the tests we run on ARM QEMU:

set_tests_properties(

I suppose there was some issue? I can try again.

It failed. From #4719 (comment) I see that drreg-test was failing for ARM already. Not sure if it fails also on real ARM machine. Is it worth fixing it right away in this PR?

@derekbruening
Copy link
Contributor

It failed. From #4719 (comment) I see that drreg-test was failing for ARM already. Not sure if it fails also on real ARM machine. Is it worth fixing it right away in this PR?

I would say no.

@abhinav92003
Copy link
Contributor Author

It failed. From #4719 (comment) I see that drreg-test was failing for ARM already. Not sure if it fails also on real ARM machine. Is it worth fixing it right away in this PR?

I would say no.

Okay. I'm not too worried about this PR as it doesn't introduce any ARM-only logic; even drreg barely has any. Also, the new ARM test has same asm as one before.

@abhinav92003 abhinav92003 merged commit e8fc651 into master May 27, 2021
@abhinav92003 abhinav92003 deleted the i3823-multi-phase-drreg branch May 27, 2021 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants