RFR: 8302328: [s390x] Simplify asm_assert definition

[Lutz Schmidt]

On Thu, 2 Mar 2023 07:49:05 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

> This PR cleanups some assert statements and specifies branch condition at calling site itself. Remaining asm_assert methods are inlined as well.

Overall, I like this change. There are some locations you have to revisit, though. Wrt inlining the as_assert* stuff see my separate comment.

src/hotspot/cpu/s390/c1_LIRAssembler_s390.cpp line 2985:

> 2983:     }
> 2984:   } else {
> 2985:     __ asm_assert(Assembler::bcondNotEqual, "unexpected null obj", __LINE__);

Condition code is from "load and test" instruction. Semantically, that is a comparison against zero. Therefore, please use the semantically correct mask bcondNotZero. Helps others to understand the code.

src/hotspot/cpu/s390/gc/g1/g1BarrierSetAssembler_s390.cpp line 180:

> 178: #ifdef ASSERT
> 179:     __ z_ltgr(Rpre_val, Rpre_val);
> 180:     __ asm_assert(Assembler::bcondNotEqual, "null oop not allowed (G1 pre)", 0x321); // Checked by caller.

Please use bcondNotZero. For reasoning, see above.

src/hotspot/cpu/s390/gc/g1/g1BarrierSetAssembler_s390.cpp line 292:

> 290: #ifdef ASSERT
> 291:     __ z_ltgr(Rnew_val, Rnew_val);
> 292:     __ asm_assert(Assembler::bcondNotEqual, "null oop not allowed (G1 post)", 0x322); // Checked by caller.

Please use bcondNotZero. For reasoning, see above.

src/hotspot/cpu/s390/macroAssembler_s390.inline.hpp line 352:

> 350:   } else {
> 351:     if (tmp != expected_size) {
> 352:       z_lgr(tmp, expected_size);

You could use lgr_if_needed() here.

src/hotspot/cpu/s390/macroAssembler_s390.inline.hpp line 353:

> 351:     if (tmp != expected_size) {
> 352:       z_lgr(tmp, expected_size);
> 353:     }

Shouldn't the else block close here? As coded, the method will have no effect for (tmp == noreg).

src/hotspot/cpu/s390/runtime_s390.cpp line 122:

> 120: #ifdef ASSERT
> 121:   __ z_ltgr(handle_exception, handle_exception);
> 122:   __ asm_assert(Assembler::bcondNotEqual, "handler must not be NULL", 0x852);

Please use bcondNotZero. For reasoning, see above.

src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 2479:

> 2477:   // Make sure that there is at least one entry in the array.
> 2478:   DEBUG_ONLY(__ z_ltgr(number_of_frames_reg, number_of_frames_reg));
> 2479:   __ asm_assert(Assembler::bcondNotEqual, "array_size must be > 0", 0x205);

Please use bcondNotZero. For reasoning, see above.

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 738:

> 736: #ifdef ASSERT
> 737:     __ z_srag(Z_R0, count, 31);  // Just leave the sign (must be zero) in Z_R0.
> 738:     __ asm_assert(Assembler::bcondEqual, "missing zero extend", 0xAFFE);

Please use bcondZero. The CC from srag is equivalent to comparing the result against zero.

src/hotspot/cpu/s390/stubRoutines_s390.cpp line 58:

> 56:     __ z_cgr(table, Z_R0);  // safety net
> 57:     __ z_bre(L);
> 58:     __ z_illtrap();

You should move z_illtrap() after asm_assert(). Not your fault!

src/hotspot/cpu/s390/stubRoutines_s390.cpp line 68:

> 66:     __ z_bre(L);
> 67:     __ z_l(Z_R0, Address(table, 4));   // Load data from memory, we know the constant we compared against.
> 68:     __ z_illtrap();

You should move z_illtrap() after asm_assert(). Not your fault!

src/hotspot/cpu/s390/stubRoutines_s390.cpp line 103:

> 101:       __ z_cgr(table, Z_R0);  // safety net
> 102:       __ z_bre(L);
> 103:       __ z_illtrap();

You should move z_illtrap() after asm_assert(). Not your fault!

src/hotspot/cpu/s390/stubRoutines_s390.cpp line 113:

> 111:       __ z_bre(L);
> 112:       __ z_lg(Z_R0, Address(table, 8));   // Load data from memory, we know the constant we compared against.
> 113:       __ z_illtrap();

You should move z_illtrap() after asm_assert(). Not your fault!

src/hotspot/share/interpreter/interp_masm.hpp line 28:

> 26: #define SHARE_INTERPRETER_INTERP_MASM_HPP
> 27: 
> 28: #include "asm/macroAssembler.inline.hpp"

All the CPU_HEADER(interp_masm) files include "asm/macroAssembler.hpp" as well. That is redundant. You could fix it, but probably with a separate PR.

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

Changes requested by lucy (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12822