RFR: JDK-8203157: Object equals abstraction for BarrierSetAssembler

[Thomas Schatzl]

Hi,

  the issue is (or actually the fix) https://bugs.openjdk.java.net/brow
se/JDK-8204861 .

Thanks,
  Thomas

On Tue, 2018-06-12 at 14:59 +0200, Roman Kennke wrote:
> jdk/submit came back with unstable (see below). Can somebody with
> access
> look what's going on?
> 
> Thanks, Roman
> 
> Build Details: 2018-06-12-1147472.roman.source
> 0 Failed Tests
> Mach5 Tasks Results Summary
> 
>     PASSED: 62
>     KILLED: 0
>     FAILED: 0
>     UNABLE_TO_RUN: 11
>     EXECUTED_WITH_FAILURE: 2
>     NA: 0
>     Build
> 
>         2 Not run
>             build_jdk_linux-linux-x64-debug-linux-x64-build-1 error
> while building, return value: 2
>             build_jdk_linux-linux-x64-open-debug-linux-x64-build-3
> error
> while building, return value: 2
> 
>     Test
> 
>         11 Not run
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_common-linux-x64-debug-
> 24
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_compiler_1-linux-x64-
> debug-27
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_compiler_2-linux-x64-
> debug-30
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_compiler_3-linux-x64-
> debug-33
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_compiler_not_xcomp-
> linux-x64-debug-36
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_gc_1-linux-x64-debug-39
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_gc_2-linux-x64-debug-42
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_gc_gcbasher-linux-x64-
> debug-45
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_gc_gcold-linux-x64-
> debug-48
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
> 
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_runtime-linux-x64-
> debug-51
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-build-1
>             See all 11...
> 
> 
> > On 06/12/2018 10:23 AM, Roman Kennke wrote:
> > > Am 12.06.2018 um 11:11 schrieb Andrew Haley:
> > > > On 06/11/2018 08:17 PM, Roman Kennke wrote:
> > > > > Am 11.06.2018 um 19:11 schrieb Andrew Haley:
> > > > > > On 06/11/2018 04:56 PM, Andrew Haley wrote:
> > > > > > > On 06/08/2018 09:17 PM, Roman Kennke wrote:
> > > > > > > > Why is it better? And how would I do that? It sounds
> > > > > > > > like a fairly
> > > > > > > > complex undertaking for a special case. Notice that if
> > > > > > > > the oop doesn't
> > > > > > > > qualify as immediate operand (quite likely for an oop?)
> > > > > > > > it used to be
> > > > > > > > moved into rscratch1 anyway a few lines below.
> > > > > > > 
> > > > > > > Sorry for the slow reply.  I'm looking now.
> > > > > > 
> > > > > > OK.  The problem is that this is a very bad code smell:
> > > > > > 
> > > > > >       case T_ARRAY:
> > > > > >         jobject2reg(opr2->as_constant_ptr()->as_jobject(),
> > > > > > rscratch1);
> > > > > >         __ cmpoop(reg1, rscratch1);
> > > > > > 
> > > > > > I can't tell that this is correct.  rscratch1 is used by
> > > > > > assembler
> > > > > > macros, and I don't know if some other GC (e.g. ZGC) might
> > > > > > need to use
> > > > > > rscratch1 inside cmpoop.  The risk here is obvious.  The
> > > > > > Right Thing
> > > > > > to do IMO is to generate a scratch register for pointer
> > > > > > comparisons.
> > > > > > 
> > > > > > Unless, I guess, we know that cmpoop never ever needs a
> > > > > > scratch
> > > > > > register for any forseeable garbage collector.
> > > > > > 
> > > > > 
> > > > > I do know that Shenandoah does not require a tmp reg. I also
> > > > > do know
> > > > > that no other collector currently needs equals-barriers at
> > > > > all.
> > > > 
> > > > So cmpoop() is literally useless.  It does nothing except add a
> > > > layer
> > > > of obfuscation in the name of some possible future collector.
> > > 
> > > The layer of abstraction is needed by Shenandoah. We need special
> > > handling for comparing oops. It is certainly not useless. Or are
> > > we
> > > talking about different issues?
> > 
> > Ah, okay.  I'm looking at
> > ShenandoahBarrierSetAssembler::obj_equals()
> > and I see that it actually has a side effect on its operands rather
> > than using scratch registers.  Ewww.  I get it now.
> > 
> > OK, I withdraw my objection.  It's very confusing code to read, but
> > it is what it is.
> > 
> 
>