Re: RFR: 8223347: Integration of Vector API (Incubator)
On Thu, 1 Oct 2020 01:01:23 GMT, Paul Sandoz wrote: >> Hi @PaulSandoz , >> >> I think it would be better to integrate it [1] in this MR. >> >> I have tested this MR on our AVX512 machines and it still crashes. >> Also, for the sake of maintenance, it seems NOT a good idea to push a >> problematic commit into the jdk main-line repo. >> >> As for the review process, I don't think it's a problem since the fix [1] is >> clear and small enough. >> >> What do you think? >> >> Thanks. >> >> [1] >> https://github.com/openjdk/panama-vector/commit/1af35c357066743935bd3f48ce3610a41761f89a > > @DamonFool I appreciate your efforts on this but i want to hold back on that > issue and follow up very quickly after > integration of this PR. This change has been through an extremely long and > arduous review process, and i want to stick > to what was reviewed and not ask reviewers to go through further cycles on > what overall is a very large change. > Unfortunately this change is in a holding pattern waiting for the CSR to be > approved thereby increasing the window > where we might find further issues (that if we had already integrated may > have been dealt with separately perhaps in a > less timely fashion with respect to that integration). Unless an issue is > extremely severe I think we should queue them > up in `panama-vector/vectorIntrinsics` (there is at least one more for ARM > SVE that is queued up). Since the issue you > describe effects one instruction, for one type, on AVX512, its impact is > limited and will be mitigated by a quick > follow up. Okay. I can understand it. Vector API is very valuable to us. Hope the follow-ups can be integrated as soon as possible. And thank you all for your great work. Best regards, Jie - PR: https://git.openjdk.java.net/jdk/pull/367
Re: RFR: 8223347: Integration of Vector API (Incubator)
On Thu, 1 Oct 2020 00:25:46 GMT, Jie Fu wrote: >> @DamonFool we can follow up later for that fix (and others in >> `vectorIntrinsics`), after this PR integrates. I don't >> want to perturb the code that has already been reviewed, which requires yet >> more additional review. > > Hi @PaulSandoz , > > I think it would be better to integrate it [1] in this MR. > > I have tested this MR on our AVX512 machines and it still crashes. > Also, for the sake of maintenance, it seems NOT a good idea to push a > problematic commit into the jdk main-line repo. > > As for the review process, I don't think it's a problem since the fix [1] is > clear and small enough. > > What do you think? > > Thanks. > > [1] > https://github.com/openjdk/panama-vector/commit/1af35c357066743935bd3f48ce3610a41761f89a @DamonFool I appreciate your efforts on this but i want to hold back on that issue and follow up very quickly after integration of this PR. This change has been through an extremely long and arduous review process, and i want to stick to what was reviewed and not ask reviewers to go through further cycles on what overall is a very large change. Unfortunately this change is in a holding pattern waiting for the CSR to be approved thereby increasing the window where we might find further issues (that if we had already integrated may have been dealt with separately perhaps in a less timely fashion with respect to that integration). Unless an issue is extremely severe I think we should queue them up in `panama-vector/vectorIntrinsics` (there is at least one more for ARM SVE that is queued up). Since the issue you describe effects one instruction, for one type, on AVX512, its impact is limited and will be mitigated by a quick follow up. - PR: https://git.openjdk.java.net/jdk/pull/367
Re: RFR: 8223347: Integration of Vector API (Incubator)
On Wed, 30 Sep 2020 18:19:53 GMT, Paul Sandoz wrote: >> Hi @PaulSandoz , >> >> This integration seems to miss >> https://github.com/openjdk/panama-vector/pull/1, which had fixed crashes on >> AVX512 >> machines. >> Thanks. > > @DamonFool we can follow up later for that fix (and others in > `vectorIntrinsics`), after this PR integrates. I don't > want to perturb the code that has already been reviewed, which requires yet > more additional review. Hi @PaulSandoz , I think it would be better to integrate it [1] in this MR. I have tested this MR on our AVX512 machines and it still crashes. Also, for the sake of maintenance, it seems NOT a good idea to push a problematic commit into the jdk main-line repo. As for the review process, I don't think it's a problem since the fix [1] is clear and small enough. What do you think? Thanks. [1] https://github.com/openjdk/panama-vector/commit/1af35c357066743935bd3f48ce3610a41761f89a - PR: https://git.openjdk.java.net/jdk/pull/367
Integrated: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package
On Mon, 7 Sep 2020 09:44:09 GMT, Philippe Marschall wrote: > Hello, newbie here > > I picked JDK-8138732 to work on because it has a "starter" label and I > believe I understand what to do. > > - I tried to update the copyright year to 2020 in every file. > - I decided to change `@since` from 9 to 16 since it is a new annotation name > in a new package. > - I tried to keep code changes to a minimum, eg not change to imports if > fully qualified class names are used instead of > imports. In some cases I did minor reordering of imports to keep them > sorted alphabetically. > - All tier1 tests pass. > - One jpackage/jlink tier2 test fails but I don't believe it is related. > - Some tier3 Swing tests fail but I don't think they are related. This pull request has now been integrated. Changeset: 2a406f3c Author:Philippe Marschall Committer: Vladimir Kozlov URL: https://git.openjdk.java.net/jdk/commit/2a406f3c Stats: 749 lines in 65 files changed: 149 ins; 149 del; 451 mod 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package Reviewed-by: dholmes, alanb, psandoz, kvn, egahlin - PR: https://git.openjdk.java.net/jdk/pull/45
Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package [v5]
On Mon, 28 Sep 2020 19:08:04 GMT, Philippe Marschall wrote: >> @marschall I will sponsor it after you integrate the latest update. > > @vnkozlov done, I hope I now made it correctly with a merge commit for the > latest merge conflict hs-tier1, hs-tier3-graal testing passed - PR: https://git.openjdk.java.net/jdk/pull/45
Re: RFR: 8223347: Integration of Vector API (Incubator)
On Wed, 30 Sep 2020 03:33:38 GMT, Jie Fu wrote: >> Build changes look ok. > > Hi @PaulSandoz , > > This integration seems to miss > https://github.com/openjdk/panama-vector/pull/1, which had fixed crashes on > AVX512 > machines. > Thanks. @DamonFool we can follow up later for that fix (and others in `vectorIntrinsics`), after this PR integrates. I don't want to perturb the code that has already been reviewed, which requires yet more additional review. - PR: https://git.openjdk.java.net/jdk/pull/367
RE: [OpenJDK 2D-Dev] RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
I filed https://bugs.openjdk.java.net/browse/JDK-8253868:
CodeSection::initialize_shared_locs buffer argument types and sizes are opaque
Paul
On 9/29/20, 9:35 AM, "Kim Barrett" wrote:
> On Sep 29, 2020, at 10:18 AM, Hohensee, Paul wrote:
> Code that calls initialize_shared_locs is inconsistent even within
itself. E.g., in c1_Compilation.cpp, we have
Agreed there seems to be a bit of a mess around that function.
> Anyway, I just wanted to make the compiler warning go away, not figure
out why the code is the way it is. Matthias and Kim would like to separate out
the CSystemColors.m patch into a separate bug, which is fine by me (see
separate email).
>
> So, for the sharedRuntime patch, would it be ok to just use
>
> short locs_buf[84];
>
> to account for possible alignment in initialize_shared_locs? I'm using 84
because 80 is exactly 5 records plus 5 sizeof(relocInfo)s, allowing for
alignment adds 3, and rounding the total array size up to a multiple of 8 adds
1.
Your analysis looks plausible. But consider instead
struct { double data[20]; } locs_buf;
and change next line to use &locs_buf
This doesn't change the size or alignment from pre-existing code. I can't
test whether this will suppress the warning, but I'm guessing it will. And
the rest
of below is off if that’s wrong.
Changing the size and type and worrying about alignment changes seems beyond
what's needed "to make the compiler warning go away, not figure out why the
code is the way it is.” I dislike making weird changes to suppress
compiler warnings,
but changing the type and size seems more weird to me here. I mean, 84
looks like
a number pulled out of a hat, unless you are going to add a bunch of
commentary
that probably really belongs in a bug report to look at this stuff more
carefully.
And file an RFE to look at initialize_shared_locs and its callers more
carefully.
RFR: 8253865: Pre-submit testing using GitHub Actions does not detect failures reliably
The pre-submit test definitions utilizing GitHub Actions can sometimes report success even when there are failing tests. The failure detection depended on make returning a non-zero exit code on failures, which doesn't seem to work. To properly determine test outcome we should check the "test-summary.txt" result file for the string "TEST SUCCESS". If it isn't found, the tests must have failed. This change also includes a reliability improvement. Here's an example of a run with the updated test definitions: https://github.com/rwestberg/jdk/actions/runs/280529475 Note! There is a failure now properly detected in langtools/tier1 on Windows (tools/javac/launcher/SourceLauncherTest.java) which is tracked by https://bugs.openjdk.java.net/browse/JDK-8249095 - could be of interest to either fix or ProblemList that one before integrating this change. - Commit messages: - Retry artifact downloading on failure - Check results and show comprehensive list of failing tests (if any) Changes: https://git.openjdk.java.net/jdk/pull/437/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=437&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253865 Stats: 108 lines in 1 file changed: 108 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/437.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/437/head:pull/437 PR: https://git.openjdk.java.net/jdk/pull/437
Integrated: 8253375: OSX build fails with Xcode 12.0 (12A7209)
On Thu, 24 Sep 2020 21:28:01 GMT, Paul Hohensee wrote: > Please review this small patch to enable the OSX build using Xcode 12.0. > > Thanks, > Paul This pull request has now been integrated. Changeset: f80a6066 Author:Paul Hohensee URL: https://git.openjdk.java.net/jdk/commit/f80a6066 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8253375: OSX build fails with Xcode 12.0 (12A7209) Replace double array with short array in AdapterHandlerLibrary::create_native_wrapper, add parens around ?: in CSystemColors:getColor Reviewed-by: prr, kbarrett, lucy - PR: https://git.openjdk.java.net/jdk/pull/348
RE: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209) [v2]
Thanks, Lutz! On 9/29/20, 10:56 PM, "build-dev on behalf of Lutz Schmidt" wrote: On Tue, 29 Sep 2020 19:33:48 GMT, Paul Hohensee wrote: >> Please review this small patch to enable the OSX build using Xcode 12.0. >> >> Thanks, >> Paul > > Paul Hohensee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev > excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since > the last revision: > - 8253375: Reverted CSystemColors.m patch, replaced sharedRuntime.cpp patch > - Merge branch 'master' into JDK-8253375 > - JDK-8253375 I just copied the declaration and oversaw that tiny little '&'. With it, sharedRuntime.cpp compiles fine. Sorry for not being careful enough. Reviewed. - Marked as reviewed by lucy (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/348
