Re: RFR (trivial): 8219519: Remove linux_sparc.ad and linux_aarch64.ad
Thanks Magnus and Andrew Dinn for your kind review.
On 2019年03月01日 22:47, Magnus Ihse Bursie wrote:
On 2019-03-01 15:39, Andrew Dinn wrote:
On 01/03/2019 14:25, Magnus Ihse Bursie wrote:
On 2019-02-27 03:25, Jie Fu wrote:
It's a bit difficult for me to test this patch since I don't have a
sparc or arm machine.
I've analyzed the adlc processing logic in
make/hotspot/gensrc/GensrcAdlc.gmk finding that ad-files under
./src/hotspot/os_cpu/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH)
are optional.
What do you mean by "optional"? The build code does this:
##
# Concatenate all ad source files into a single file, which will be fed to
# adlc.
...
AD_SRC_FILES := $(call uniq, $(wildcard $(foreach d, $(AD_SRC_ROOTS), \
$d/cpu/$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_CPU).ad \
$d/cpu/$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_CPU_ARCH).ad \
$d/os_cpu/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH).ad
\
)))
so it will definitely pick up both those files and use it in creating
the concatenated ad file.
That's interesting because Pengfei Li claims he applied the patch and
successfully built OpenJDK on AArch64.
https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2019-February/006975.html
Does the build system actually need those files to exist when it builds
the concatenated file?
No, the build system does not "need" it. If it is not there, it is not
included (nor reported MIA), but if it is there, it is included.
That being said, maybe this is not the correct behavior.
Well, something sounds fishy.
I see that the linux_sparc.ad file is essentially empty, so you can
probably remove that. The aarch64 file otoh seems to contain valid code.
I would not presume that you can just remove it!
He is ok to remove it as far as any contents are concerned. Indeed, I
told him this was ok in a review in the above thread after Pengfei
reported that OpenJDK built without the file being present.
As to the contents, the encoding defined in that file is completely
redundant (I don't really know how it got there as I don't believe it
was ever used)
Ok, it might very well be the case that the file is not needed since
it's contents is redundant. I can't say anything about that; that's
the domain of the adlc experts. However, it is incorrect to claim that
the build does not use file in question. But from the build PoV, it's
perfectly fine to remove it if it's not needed. But just not on the
grounds that it is not used by the build system!
/Magnus
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (trivial): 8219519: Remove linux_sparc.ad and linux_aarch64.ad
That's great! Thanks Adrian. On 2019年03月01日 22:29, John Paul Adrian Glaubitz wrote: Hello! On 3/1/19 3:25 PM, Magnus Ihse Bursie wrote: It's a bit difficult for me to test this patch since I don't have a sparc or arm machine. Both SPARC and AArch64 machines running Linux can be accessed through the gcc compile farm. Any open source developer can request an account for these machines. See: https://gcc.gnu.org/wiki/CompileFarm https://cfarm.tetaneutral.net/machines/list/ I'm admin for the sparc64 box running Linux in case someone needs any particular package to be installed. Thanks, Adrian
Re: RFR (trivial): 8219519: Remove linux_sparc.ad and linux_aarch64.ad
On 2019-03-01 15:39, Andrew Dinn wrote:
On 01/03/2019 14:25, Magnus Ihse Bursie wrote:
On 2019-02-27 03:25, Jie Fu wrote:
It's a bit difficult for me to test this patch since I don't have a
sparc or arm machine.
I've analyzed the adlc processing logic in
make/hotspot/gensrc/GensrcAdlc.gmk finding that ad-files under
./src/hotspot/os_cpu/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH)
are optional.
What do you mean by "optional"? The build code does this:
##
# Concatenate all ad source files into a single file, which will be fed to
# adlc.
...
AD_SRC_FILES := $(call uniq, $(wildcard $(foreach d, $(AD_SRC_ROOTS), \
$d/cpu/$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_CPU).ad \
$d/cpu/$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_CPU_ARCH).ad \
$d/os_cpu/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH).ad
\
)))
so it will definitely pick up both those files and use it in creating
the concatenated ad file.
That's interesting because Pengfei Li claims he applied the patch and
successfully built OpenJDK on AArch64.
https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2019-February/006975.html
Does the build system actually need those files to exist when it builds
the concatenated file?
No, the build system does not "need" it. If it is not there, it is not
included (nor reported MIA), but if it is there, it is included.
That being said, maybe this is not the correct behavior.
Well, something sounds fishy.
I see that the linux_sparc.ad file is essentially empty, so you can
probably remove that. The aarch64 file otoh seems to contain valid code.
I would not presume that you can just remove it!
He is ok to remove it as far as any contents are concerned. Indeed, I
told him this was ok in a review in the above thread after Pengfei
reported that OpenJDK built without the file being present.
As to the contents, the encoding defined in that file is completely
redundant (I don't really know how it got there as I don't believe it
was ever used)
Ok, it might very well be the case that the file is not needed since
it's contents is redundant. I can't say anything about that; that's the
domain of the adlc experts. However, it is incorrect to claim that the
build does not use file in question. But from the build PoV, it's
perfectly fine to remove it if it's not needed. But just not on the
grounds that it is not used by the build system!
/Magnus
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (trivial): 8219519: Remove linux_sparc.ad and linux_aarch64.ad
On 01/03/2019 14:25, Magnus Ihse Bursie wrote:
> On 2019-02-27 03:25, Jie Fu wrote:
>> It's a bit difficult for me to test this patch since I don't have a
>> sparc or arm machine.
>> I've analyzed the adlc processing logic in
>> make/hotspot/gensrc/GensrcAdlc.gmk finding that ad-files under
>> ./src/hotspot/os_cpu/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH)
>> are optional.
> What do you mean by "optional"? The build code does this:
>
>
> ##
> # Concatenate all ad source files into a single file, which will be fed to
> # adlc.
>
> ...
>
> AD_SRC_FILES := $(call uniq, $(wildcard $(foreach d, $(AD_SRC_ROOTS), \
> $d/cpu/$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_CPU).ad \
> $d/cpu/$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_CPU_ARCH).ad \
>
> $d/os_cpu/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH).ad
> \
> )))
>
> so it will definitely pick up both those files and use it in creating
> the concatenated ad file.
That's interesting because Pengfei Li claims he applied the patch and
successfully built OpenJDK on AArch64.
https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2019-February/006975.html
Does the build system actually need those files to exist when it builds
the concatenated file?
> That being said, maybe this is not the correct behavior.
Well, something sounds fishy.
> I see that the linux_sparc.ad file is essentially empty, so you can
> probably remove that. The aarch64 file otoh seems to contain valid code.
> I would not presume that you can just remove it!
He is ok to remove it as far as any contents are concerned. Indeed, I
told him this was ok in a review in the above thread after Pengfei
reported that OpenJDK built without the file being present.
As to the contents, the encoding defined in that file is completely
redundant (I don't really know how it got there as I don't believe it
was ever used)
regards,
Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR (trivial): 8219519: Remove linux_sparc.ad and linux_aarch64.ad
Hello! On 3/1/19 3:25 PM, Magnus Ihse Bursie wrote: >> It's a bit difficult for me to test this patch since I don't have a sparc or >> arm machine. Both SPARC and AArch64 machines running Linux can be accessed through the gcc compile farm. Any open source developer can request an account for these machines. See: > https://gcc.gnu.org/wiki/CompileFarm > https://cfarm.tetaneutral.net/machines/list/ I'm admin for the sparc64 box running Linux in case someone needs any particular package to be installed. Thanks, Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaub...@debian.org `. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: RFR (trivial): 8219519: Remove linux_sparc.ad and linux_aarch64.ad
On 2019-02-27 03:25, Jie Fu wrote: Hi Tobias, Thanks a lot for your review. It's a bit difficult for me to test this patch since I don't have a sparc or arm machine. I've analyzed the adlc processing logic in make/hotspot/gensrc/GensrcAdlc.gmk finding that ad-files under ./src/hotspot/os_cpu/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH) are optional. What do you mean by "optional"? The build code does this: ## # Concatenate all ad source files into a single file, which will be fed to # adlc. ... AD_SRC_FILES := $(call uniq, $(wildcard $(foreach d, $(AD_SRC_ROOTS), \ $d/cpu/$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_CPU).ad \ $d/cpu/$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_CPU_ARCH).ad \ $d/os_cpu/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH)/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ARCH).ad \ ))) so it will definitely pick up both those files and use it in creating the concatenated ad file. That being said, maybe this is not the correct behavior. I see that the linux_sparc.ad file is essentially empty, so you can probably remove that. The aarch64 file otoh seems to contain valid code. I would not presume that you can just remove it! /Magnus Since both linux_sparc.ad and linux_aarch64.ad are useless for the generation of C2, it would be better to remove them. I'll try my best to test it. By the way, I really appreciate If someone with sparc or aarch64 development environment could help to verify this change. Thanks in advance. Best regards, Jie On 2019/2/27 上午12:51, Tobias Hartmann wrote: Hi Jie, this looks good to me assuming that you have tested the change on these platforms. Best regards, Tobias On 21.02.19 10:35, Jie Fu wrote: Hi all, The following two source files are useless for the generation of C2 and should be removed. 1) ./src/hotspot/os_cpu/linux_sparc/linux_sparc.ad 2) ./src/hotspot/os_cpu/linux_aarch64/linux_aarch64.ad Bug: https://bugs.openjdk.java.net/browse/JDK-8219519 Webrev: http://cr.openjdk.java.net/~jiefu/8219519/webrev.00/ Could you please review it? Thanks a lot. Best regards, Jie
