[8u] RFR 8248901: Signed immediate support in .../share/assembler.hpp is broken.

[Andrew Hughes]

On 10:48 Thu 24 Jun     , Andrew Dinn wrote:
> On 23/06/2021 17:47, Andrew Hughes wrote:
> > This seems to be missing changes that were made for 11u. With backports,
> > we should work from the closest version that has the fix, so to try and
> > avoid repeating analysis that has already been done in earlier reviews.
> > For 8u, this should mean working from 11u, unless there is a good reason
> > why the fix is not applicable there.
> 
> All that is missing is
> 
>   i) the arm.ad change (this port does not exist in jdk8u)
>   ii) use of the precond macro (replaced with assert)
>

Compared to the 11u version, there are also changes to
src/hotspot/cpu/ppc/macroAssembler_ppc.cpp,
src/hotspot/cpu/sparc/assembler_sparc.hpp and
src/hotspot/share/utilities/debug.hpp that are absent.

While I'm ok with omitting changes from the 11u backport with good reason,
tt wasn't clear to me in the initial post that the 11u version had even
been considered in backporting this change.

> > In the 11u changeset:
> > 
> > https://hg.openjdk.java.net/jdk-updates/jdk11u/rev/d21f0f95e6b4
> > 
> > the precond macros are introduced from JDK-8223140, which, given their
> > simplicity, I think makes sense and will help any future backports that
> > use these macros.
> 
> I'm not sure this is clear-cut. That patch changes code in the opto
> compiler. While it looks like those changes would apply cleanly and
> correctly, backporting them just to obtain a macro equivalent to the assert
> used by Zhengyu's patch is a risk. Replacing the calls to precond with
> equivalent calls to assert is clearly a lower risk. It also does not stop
> JDK-8223140 from being back-ported later should need for that arise.
>

Sorry if I was unclear here. I'm not suggesting backporting the whole
of 8223140. I'm suggesting to follow 11u in adding this hunk:

--- a/src/hotspot/share/utilities/debug.hpp	Sat Jul 04 08:18:17 2020 +0800
+++ b/src/hotspot/share/utilities/debug.hpp	Mon Jul 06 21:29:51 2020 +0200
@@ -67,6 +67,9 @@
 // For backward compatibility.
 #define assert(p, ...) vmassert(p, __VA_ARGS__)
 
+#define precond(p)   assert(p, "precond")
+#define postcond(p)  assert(p, "postcond")
+
 #ifndef ASSERT
 #define vmassert_status(p, status, msg)
 #else

See:

https://hg.openjdk.java.net/jdk-updates/jdk11u/rev/d21f0f95e6b4

This should achieve the same as replacing precond with assert, but also provide
precond for future backports.

While I won't insist on this, I do think it odd if 8u & 11u choose different
paths on this without good reason.

> > It also includes the ppc64 fix, JDK-8215144, though uncredited, and some
> > sparc changes (there is no sparc port in trunk).
> 
> If the ppc patch is needed then it can be back-ported independent of this
> one.
>

The change removes several functions altogether. It is possible to tell just
by simple inspection if this will break the builds on other platforms.

The following are entirely removed and not replaced:

is_simm5
is_simm11
is_simm12
is_simm13
is_simm26
min_simm
max_simm
min_simm13
min_simm16

>From a search of the code, it appears only is_simm26, min_simm, max_simm
and min_simm16 are actually safe to be completely removed in 8u.

is_simm5, is_simm11, is_simm12, is_simm13 and min_simm13 are indeed all
used by the SPARC assembler code, as the 11u review already identified and
fixed. This is clearly not the case in trunk, as the SPARC port has been
removed.

It looks like the ppc64 change is unneeded in 8u, as we don't have
JDK-8144019: "PPC64 C1: Introduce Client Compiler" that introduces the
is_simm13 call (and, for clarity, no I'm not suggesting we backport it ;) )

However, I can't approve this, knowing it will break SPARC. So, I
suggest either those functions are left untouched or (preferably) we
follow 11u in localising them to the only consumer, the SPARC port.

Thanks,
-- 
Andrew :)
Pronouns: he / him or they / them
Senior Free Java Software Engineer
OpenJDK Package Owner
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222