[lworld] RFR: 8311219: [lworld] VM option "InlineFieldMaxFlatSize" cannot work well [v3]

[Xiaohong Gong]

On Wed, 6 Sep 2023 11:09:46 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> Yes, please include it in this PR but on second thought, the proper fix should look like this:
> 
> ```
> -  assert(!null_free || vt->is_allocated(&gvn), "inline type should be allocated");
> +  assert(vt->is_allocated(&gvn) || (null_free && !vk->is_initialized()), "inline type should be allocated");
> ```
> 
> All InlineTypeNodes returned by this method should be allocated except for null free with uninitialized value class (because we can't load the default oop for those).

New assertion makes sense to me. I will update the change in this PR. Thanks so much for the fixing! 

> Testing found some additional issues triggered by your change. Please include below fixes as well.
> 
> We hit the `"Should have been buffered"` assert because the verification code is too strong. InlineType users don't require buffering because they are removed as well. In addition, the `u->Opcode() != Op_Return || !tf()->returns_inline_type_as_fields()` condition is not required.
> 
> ```
> --- a/src/hotspot/share/opto/compile.cpp
> +++ b/src/hotspot/share/opto/compile.cpp
> @@ -2045,16 +2045,10 @@ void Compile::process_inline_types(PhaseIterGVN &igvn, bool remove) {
>  #ifdef ASSERT
>          // Verify that inline type is buffered when replacing by oop
>          else if (u->is_InlineType()) {
> -          InlineTypeNode* vt2 = u->as_InlineType();
> -          for (uint i = 0; i < vt2->field_count(); ++i) {
> -            if (vt2->field_value(i) == vt && !vt2->field_is_flattened(i)) {
> -              // Use in non-flat field
> -              must_be_buffered = true;
> -            }
> -          }
> +          // InlineType uses don't need buffering because they are about to be replaced as well
>          } else if (u->is_Phi()) {
>            // TODO 8302217 Remove this once InlineTypeNodes are reliably pushed through
> -        } else if (u->Opcode() != Op_Return || !tf()->returns_inline_type_as_fields()) {
> +        } else {
>            must_be_buffered = true;
>          }
>          if (must_be_buffered && !vt->is_allocated(&igvn)) {
> ```

Also thanks for this fixing! Applying such patch can clean new involved tests failures with `-XX:InlineFieldMaxFlatSize=0`. But I don't quite understand it well. Following is my questions:
1. This change seems extended the `buffer check` for users besides `InlineType`, didn't it? So if the current `InlineTypeNode` is used in a method call which expects the `InlineType` is scalarized as arguments/return, is the buffer for the `InlineType` still needed?
2. Conversely, if an `InlineType` is a non-flattened field of another `InlineType`, why is the buffer not needed although the user `InlineType` will be replaced with its oop? Per my understanding, the oop is the class instance of the `InlineType`, which also contains the fields.

With another thinking, I assumed maybe all the `InlineType` have been scalarized to their users if it can be before this final replacement (i.e. replacement with its oop). So for almost all cases, the remaining `InlineTypeNode` must be buffered at this point, right?

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

PR Comment: https://git.openjdk.org/valhalla/pull/888#issuecomment-1709438654