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

Liveness: improve for register allocation #469

Merged
merged 2 commits into from
Jun 5, 2023
Merged

Liveness: improve for register allocation #469

merged 2 commits into from
Jun 5, 2023

Conversation

vbgl
Copy link
Member

@vbgl vbgl commented Jun 1, 2023

Fixes #455

@eponier
Copy link
Contributor

eponier commented Jun 1, 2023

I don't get the idea of the second commit. I have the impression that we are less precise than before, and it gives better results?

@vbgl
Copy link
Member Author

vbgl commented Jun 1, 2023

Yes, I don’t really understand why we had this special case…

@vbgl
Copy link
Member Author

vbgl commented Jun 1, 2023

Without this change, when there is x = y; with y live afterwards and x dead, we tell the register allocation that it is OK to allocate both x and y to the same register. Unfortunately, the checker does not know about the semantics of move instructions.

Maybe I could fix that instead…

@vbgl vbgl marked this pull request as draft June 1, 2023 13:50
@vbgl
Copy link
Member Author

vbgl commented Jun 1, 2023

With this patch, the compiler would generate a dead mov, e.g., movq %rdi, %rax. Without this patch but with an improved checker, we would generate a silly dead mov, e.g., movq %rdi, %rdi.

Maybe it’s better to just crash…

@vbgl vbgl marked this pull request as ready for review June 1, 2023 15:31
@vbgl
Copy link
Member Author

vbgl commented Jun 1, 2023

I think this change is good: it simplifies code that is unnecessarily complex.

bgregoir
bgregoir approved these changes Jun 5, 2023
View reviewed changes
compiler/safetylib/domains/safetyProduct.ml
let asmOp = Arch_extra.asm_opI X86_arch_full.X86_core.asm_e in
FSPa.fs_pa_make asmOp is_move_op PW.main
FSPa.fs_pa_make asmOp PW.main

(* We compute the reflexive and transitive clojure of dp *)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not so frequent that I am able to find a typo in a comment (clojure -> closure) :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

vbgl and others added 2 commits June 5, 2023 08:48
@vbgl
Printer.pp_dvar: print variable with definition location
e7296a1 
unless it is dummy
@vbgl @bgregoir
Liveness: improve for register allocation
e545823 
This somehow reverts commit 72d20a6

Co-authored-by: Benjamin Grēgoire <Benjamin.Gregoire@inria.fr>
@vbgl vbgl merged commit 7896310 into main Jun 5, 2023
1 check passed
@vbgl vbgl deleted the fix-455 branch June 5, 2023 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgregoir bgregoir bgregoir approved these changes

Assignees
No one assigned
Labels
bug register-allocation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Register allocation bug with instrinsics
3 participants
@vbgl @eponier @bgregoir