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

Code cleanups in asmcomp/s390x #908

Merged
merged 3 commits into from Dec 30, 2016

Conversation

Projects
None yet
3 participants
@gasche
Member

gasche commented Nov 11, 2016

I prepared this PR yesterday while working on #903, to try to make the emit.mlp code more uniform and better understand the s390x PIC-code provisions.

We may want to throw some or all of it away, based on @xavierleroy's reasonable "if it ain't broke..." principles:

There is no urgent need to make things more uniform across platforms as @gasche suggests: nothing is really broken, and changing code generators that have very few users (like ARM64 or S390x) can create nasty bugs that we detect late.

On the other hand, @xavierleroy also pointed out:

I'm actually more concerned that in the S390x code emitter we still have places where brasl is used directly instead of emit_call. In other words, consistency within one code emitter is more important than consistency across emitters.

In the present PR, there is a bit of consistency stuff on s390x and arm, on the newlines, but also a factorization of the PIC-dependent code in s390x that in particular removes all calls to brasl outside emit_call, except the one in Lsetuptrap that branches to a local label.

I'm thinking of removing the arm part of the PR, so that it can be successfully rebranded as "make s390x more consistent" -- and also can be tested as a single patch by s390x users if they are interested.

@gasche gasche changed the title from Code cleanups in asmcomp/{arm,s390x} to Code cleanups in asmcomp/s390x Nov 11, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 11, 2016

Member

I removed the arm part of the PR as planned. It can still be found at gasche@51390e9.

Member

gasche commented Nov 11, 2016

I removed the arm part of the PR as planned. It can still be found at gasche@51390e9.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Nov 14, 2016

Contributor

Globally speaking I find half of your changes well taken and half entirely gratuitous, e.g. emitting a newline where there was none and conversely. So, I don't know wher we go from here.

Contributor

xavierleroy commented Nov 14, 2016

Globally speaking I find half of your changes well taken and half entirely gratuitous, e.g. emitting a newline where there was none and conversely. So, I don't know wher we go from here.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 14, 2016

Member

But the gratuitous changes come for free! :-)

What is very easy is to just drop the last commit that inserts some gratuitous newline (I'll update the PR right now). I don't know if you consider the first commit gratuitous as well: it is on its own, but it helps in the refactoring that follows.

Member

gasche commented Nov 14, 2016

But the gratuitous changes come for free! :-)

What is very easy is to just drop the last commit that inserts some gratuitous newline (I'll update the PR right now). I don't know if you consider the first commit gratuitous as well: it is on its own, but it helps in the refactoring that follows.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 28, 2016

Contributor

I think the whole thing seems ok; even though I agree consistency within each individual emitter is important, it seems good to strive for a situation where the emitters are as consistent between each other as possible also. This helps for changes such as Spacetime, debugging information emission, etc.

I've read this patch carefully this morning and it looks correct to me.

Contributor

mshinwell commented Dec 28, 2016

I think the whole thing seems ok; even though I agree consistency within each individual emitter is important, it seems good to strive for a situation where the emitters are as consistent between each other as possible also. This helps for changes such as Spacetime, debugging information emission, etc.

I've read this patch carefully this morning and it looks correct to me.

Show outdated Hide outdated asmcomp/s390x/emit.mlp
@@ -94,6 +100,14 @@ let emit_stack r =
| _ -> fatal_error "Emit.emit_stack"
(* Output a load to a global address *)

This comment has been minimized.

@mshinwell

mshinwell Dec 28, 2016

Contributor

"Load the address of a global symbol into a register"

@mshinwell

mshinwell Dec 28, 2016

Contributor

"Load the address of a global symbol into a register"

This comment has been minimized.

@gasche

gasche Dec 28, 2016

Member

The style (* Output a ... *) is more consistent with the other comments of emit_ functions in the file, but I agree the wording was wrong. I chose the still awkward but more correct "Output a load of the address of a global symbol".

@gasche

gasche Dec 28, 2016

Member

The style (* Output a ... *) is more consistent with the other comments of emit_ functions in the file, but I agree the wording was wrong. I chose the still awkward but more correct "Output a load of the address of a global symbol".

@mshinwell mshinwell added the approved label Dec 28, 2016

@mshinwell mshinwell added this to the 4.05.0 milestone Dec 28, 2016

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 30, 2016

Contributor

No other comments, so merging.

Contributor

mshinwell commented Dec 30, 2016

No other comments, so merging.

@mshinwell mshinwell merged commit e97173d into ocaml:trunk Dec 30, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment