RFR: 8264188: Improve handling of assembly files in the JDK

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8264188: Improve handling of assembly files in the JDK

Magnus Ihse Bursie-3
We have a handful of assembly files in the JDK. They have long been left aside, with a "if it ain't broken, don't fix it" attitude.

In the current panama-vector, there is a lot more assembly files incoming, including for the Windows platforrm, which has not existed for a long time in the JDK.

It is time to give assembly files some more love and care. This patch cleans up the handling in the build system, and it unifies between .s and .S files.

For historical reasons, .s has been the suffix used in the posix world to signify assembly output as generated by a compiler, and .S to signify "hand-written" precious assembly. One effect of this is that gcc and clang will run the preprocessor on files named .S but not on files named .s.

All our files are "hand-written" in this sense, and should have the .S suffix. But not all had. On mac, it was even worse, where the files were named .s but the option `-x assembler-with-cpp` was used to force clang to treat them as .S files instead... This change however made the preprocesser try to parse comments of the form
# if (a) {
as preprocessor directives, and balk at them. In one of the files, I had to wrap this in preprocessor comments (`/* ... */`).

We also had inconsistent handling on dependencies. For preprocessed assembly files, it really makes sense to have dependency tracking, exactly as for C/C++ files. Now the dependency tracking in NativeCompilation is simplified, and applies to all files. (The sole exception is Windows assembly, since masm is unable to output dependency information, even though it is able to include files :-().

This patch has been partly written by Sandhya Viswanathan <[hidden email]> for the panama-vector repo.

-------------

Commit messages:
 - Fix comments that trip up the preprocessor
 - Transplant over more fixes from panama-vector
 - -Ta needs to be prefixed to source file
 - Remove support for non-preprocessed assembly files in gcc/clang
 - Rename all *.s files to *.S
 - Port over initial fixes from panama-vector

Changes: https://git.openjdk.java.net/jdk/pull/3198/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3198&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264188
  Stats: 79 lines in 12 files changed: 33 ins; 9 del; 37 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3198.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3198/head:pull/3198

PR: https://git.openjdk.java.net/jdk/pull/3198
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264188: Improve handling of assembly files in the JDK

Erik Joelsson-2
On Thu, 25 Mar 2021 16:25:32 GMT, Magnus Ihse Bursie <[hidden email]> wrote:

> We have a handful of assembly files in the JDK. They have long been left aside, with a "if it ain't broken, don't fix it" attitude.
>
> In the current panama-vector, there is a lot more assembly files incoming, including for the Windows platforrm, which has not existed for a long time in the JDK.
>
> It is time to give assembly files some more love and care. This patch cleans up the handling in the build system, and it unifies between .s and .S files.
>
> For historical reasons, .s has been the suffix used in the posix world to signify assembly output as generated by a compiler, and .S to signify "hand-written" precious assembly. One effect of this is that gcc and clang will run the preprocessor on files named .S but not on files named .s.
>
> All our files are "hand-written" in this sense, and should have the .S suffix. But not all had. On mac, it was even worse, where the files were named .s but the option `-x assembler-with-cpp` was used to force clang to treat them as .S files instead... This change however made the preprocesser try to parse comments of the form
> # if (a) {
> as preprocessor directives, and balk at them. In one of the files, I had to wrap this in preprocessor comments (`/* ... */`).
>
> We also had inconsistent handling on dependencies. For preprocessed assembly files, it really makes sense to have dependency tracking, exactly as for C/C++ files. Now the dependency tracking in NativeCompilation is simplified, and applies to all files. (The sole exception is Windows assembly, since masm is unable to output dependency information, even though it is able to include files :-().
>
> This patch has been partly written by Sandhya Viswanathan <[hidden email]> for the panama-vector repo.

make/autoconf/toolchain.m4 line 887:

> 885:       # On windows, the assember is "ml.exe". We currently don't need this so
> 886:       # do not require.
> 887:       if test "x$OPENJDK_TARGET_CPU_BITS" = "x64"; then

If this is for the BUILD_AS, shouldn't we be checking OPENJDK_BUILD_CPU_BITS?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3198
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264188: Improve handling of assembly files in the JDK

David Holmes-2
In reply to this post by Magnus Ihse Bursie-3
On Thu, 25 Mar 2021 16:25:32 GMT, Magnus Ihse Bursie <[hidden email]> wrote:

> We have a handful of assembly files in the JDK. They have long been left aside, with a "if it ain't broken, don't fix it" attitude.
>
> In the current panama-vector, there is a lot more assembly files incoming, including for the Windows platforrm, which has not existed for a long time in the JDK.
>
> It is time to give assembly files some more love and care. This patch cleans up the handling in the build system, and it unifies between .s and .S files.
>
> For historical reasons, .s has been the suffix used in the posix world to signify assembly output as generated by a compiler, and .S to signify "hand-written" precious assembly. One effect of this is that gcc and clang will run the preprocessor on files named .S but not on files named .s.
>
> All our files are "hand-written" in this sense, and should have the .S suffix. But not all had. On mac, it was even worse, where the files were named .s but the option `-x assembler-with-cpp` was used to force clang to treat them as .S files instead... This change however made the preprocesser try to parse comments of the form
> # if (a) {
> as preprocessor directives, and balk at them. In one of the files, I had to wrap this in preprocessor comments (`/* ... */`).
>
> We also had inconsistent handling on dependencies. For preprocessed assembly files, it really makes sense to have dependency tracking, exactly as for C/C++ files. Now the dependency tracking in NativeCompilation is simplified, and applies to all files. (The sole exception is Windows assembly, since masm is unable to output dependency information, even though it is able to include files :-().
>
> This patch has been partly written by Sandhya Viswanathan <[hidden email]> for the panama-vector repo.

Hi Magnus,

The renaming seems reasonable to me, but I think these files are of most interest to the compiler folk so I've added hotspot-compiler-dev to the cc list.

Can't comment on the build changes in detail - they seem reasonable other than Erik's queries about selecting 32-bit versus 64-bit based on the host or the target. I'm assuming the host must be 64-bit to build for 64-bit.

Thanks,
David

-------------

PR: https://git.openjdk.java.net/jdk/pull/3198
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264188: Improve handling of assembly files in the JDK [v2]

Magnus Ihse Bursie-3
In reply to this post by Magnus Ihse Bursie-3
> We have a handful of assembly files in the JDK. They have long been left aside, with a "if it ain't broken, don't fix it" attitude.
>
> In the current panama-vector, there is a lot more assembly files incoming, including for the Windows platforrm, which has not existed for a long time in the JDK.
>
> It is time to give assembly files some more love and care. This patch cleans up the handling in the build system, and it unifies between .s and .S files.
>
> For historical reasons, .s has been the suffix used in the posix world to signify assembly output as generated by a compiler, and .S to signify "hand-written" precious assembly. One effect of this is that gcc and clang will run the preprocessor on files named .S but not on files named .s.
>
> All our files are "hand-written" in this sense, and should have the .S suffix. But not all had. On mac, it was even worse, where the files were named .s but the option `-x assembler-with-cpp` was used to force clang to treat them as .S files instead... This change however made the preprocesser try to parse comments of the form
> # if (a) {
> as preprocessor directives, and balk at them. In one of the files, I had to wrap this in preprocessor comments (`/* ... */`).
>
> We also had inconsistent handling on dependencies. For preprocessed assembly files, it really makes sense to have dependency tracking, exactly as for C/C++ files. Now the dependency tracking in NativeCompilation is simplified, and applies to all files. (The sole exception is Windows assembly, since masm is unable to output dependency information, even though it is able to include files :-().
>
> This patch has been partly written by Sandhya Viswanathan <[hidden email]> for the panama-vector repo.

Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision:

  Use OPENJDK_BUILD_CPU_BITS instead

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3198/files
  - new: https://git.openjdk.java.net/jdk/pull/3198/files/62fe412b..effd75a9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3198&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3198&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3198.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3198/head:pull/3198

PR: https://git.openjdk.java.net/jdk/pull/3198
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264188: Improve handling of assembly files in the JDK [v2]

Magnus Ihse Bursie-3
In reply to this post by Erik Joelsson-2
On Thu, 25 Mar 2021 16:47:48 GMT, Erik Joelsson <[hidden email]> wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Use OPENJDK_BUILD_CPU_BITS instead
>
> make/autoconf/toolchain.m4 line 887:
>
>> 885:       # On windows, the assember is "ml.exe". We currently don't need this so
>> 886:       # do not require.
>> 887:       if test "x$OPENJDK_TARGET_CPU_BITS" = "x64"; then
>
> If this is for the BUILD_AS, shouldn't we be checking OPENJDK_BUILD_CPU_BITS?

Yes, technically you are correct. I will update. It is a bit of an academic exercise though, since this is not being used. But still, it's good to have for consistency.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3198
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264188: Improve handling of assembly files in the JDK [v2]

Erik Joelsson-2
In reply to this post by Magnus Ihse Bursie-3
On Mon, 29 Mar 2021 10:48:55 GMT, Magnus Ihse Bursie <[hidden email]> wrote:

>> We have a handful of assembly files in the JDK. They have long been left aside, with a "if it ain't broken, don't fix it" attitude.
>>
>> In the current panama-vector, there is a lot more assembly files incoming, including for the Windows platforrm, which has not existed for a long time in the JDK.
>>
>> It is time to give assembly files some more love and care. This patch cleans up the handling in the build system, and it unifies between .s and .S files.
>>
>> For historical reasons, .s has been the suffix used in the posix world to signify assembly output as generated by a compiler, and .S to signify "hand-written" precious assembly. One effect of this is that gcc and clang will run the preprocessor on files named .S but not on files named .s.
>>
>> All our files are "hand-written" in this sense, and should have the .S suffix. But not all had. On mac, it was even worse, where the files were named .s but the option `-x assembler-with-cpp` was used to force clang to treat them as .S files instead... This change however made the preprocesser try to parse comments of the form
>> # if (a) {
>> as preprocessor directives, and balk at them. In one of the files, I had to wrap this in preprocessor comments (`/* ... */`).
>>
>> We also had inconsistent handling on dependencies. For preprocessed assembly files, it really makes sense to have dependency tracking, exactly as for C/C++ files. Now the dependency tracking in NativeCompilation is simplified, and applies to all files. (The sole exception is Windows assembly, since masm is unable to output dependency information, even though it is able to include files :-().
>>
>> This patch has been partly written by Sandhya Viswanathan <[hidden email]> for the panama-vector repo.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision:
>
>   Use OPENJDK_BUILD_CPU_BITS instead

Marked as reviewed by erikj (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3198
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264188: Improve handling of assembly files in the JDK [v2]

Magnus Ihse Bursie
Anyone in hotspot-compiler who wants to have a look at this?

/Magnus

On 2021-03-29 14:47, Erik Joelsson wrote:

> On Mon, 29 Mar 2021 10:48:55 GMT, Magnus Ihse Bursie <[hidden email]> wrote:
>
>>> We have a handful of assembly files in the JDK. They have long been left aside, with a "if it ain't broken, don't fix it" attitude.
>>>
>>> In the current panama-vector, there is a lot more assembly files incoming, including for the Windows platforrm, which has not existed for a long time in the JDK.
>>>
>>> It is time to give assembly files some more love and care. This patch cleans up the handling in the build system, and it unifies between .s and .S files.
>>>
>>> For historical reasons, .s has been the suffix used in the posix world to signify assembly output as generated by a compiler, and .S to signify "hand-written" precious assembly. One effect of this is that gcc and clang will run the preprocessor on files named .S but not on files named .s.
>>>
>>> All our files are "hand-written" in this sense, and should have the .S suffix. But not all had. On mac, it was even worse, where the files were named .s but the option `-x assembler-with-cpp` was used to force clang to treat them as .S files instead... This change however made the preprocesser try to parse comments of the form
>>> # if (a) {
>>> as preprocessor directives, and balk at them. In one of the files, I had to wrap this in preprocessor comments (`/* ... */`).
>>>
>>> We also had inconsistent handling on dependencies. For preprocessed assembly files, it really makes sense to have dependency tracking, exactly as for C/C++ files. Now the dependency tracking in NativeCompilation is simplified, and applies to all files. (The sole exception is Windows assembly, since masm is unable to output dependency information, even though it is able to include files :-().
>>>
>>> This patch has been partly written by Sandhya Viswanathan <[hidden email]> for the panama-vector repo.
>> Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision:
>>
>>    Use OPENJDK_BUILD_CPU_BITS instead
> Marked as reviewed by erikj (Reviewer).
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3198