RFR: 8298952: All nodes should have type(n) == Value(n) after IGVN

[Vladimir Kozlov]

On Fri, 27 Jan 2023 08:31:18 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/parse1.cpp line 547:
>> 
>>> 545:   }
>>> 546: 
>>> 547:   gvn().set_type(root(), root()->bottom_type());
>> 
>> I am not sure about this. This `gvn` is local to `Parse` which is used separately for each parsed inlined method and intrinsic.
>
> @vnkozlov I moved this code out to here:
> https://github.com/openjdk/jdk/blob/6ce2768c37713bb342608f7017292bec74a060a6/src/hotspot/share/opto/compile.cpp#L760
> The issue was that other paths did not have the type of `root` set.
> 
> Are you sure that the `gvn` is local?
> https://github.com/openjdk/jdk/blob/6ce2768c37713bb342608f7017292bec74a060a6/src/hotspot/share/opto/parse.hpp#L151
> https://github.com/openjdk/jdk/blob/6ce2768c37713bb342608f7017292bec74a060a6/src/hotspot/share/opto/graphKit.hpp#L58-L63
> https://github.com/openjdk/jdk/blob/6ce2768c37713bb342608f7017292bec74a060a6/src/hotspot/share/opto/graphKit.cpp#L56-L78
> It looks like the `gvn` in `Parse` is the same as `C->initial_gvn()`
> 
> And we set that just before we create the `ParseGenerator` (osr / intrinsic / inline):
> https://github.com/openjdk/jdk/blob/6ce2768c37713bb342608f7017292bec74a060a6/src/hotspot/share/opto/compile.cpp#L715
> https://github.com/openjdk/jdk/blob/6ce2768c37713bb342608f7017292bec74a060a6/src/hotspot/share/opto/compile.cpp#L725
> 
> So instead of only setting the type in `Parse::Parse`, I now set it for all of those cases:
> https://github.com/openjdk/jdk/blob/6ce2768c37713bb342608f7017292bec74a060a6/src/hotspot/share/opto/compile.cpp#L760
> 
> I can also set the type both out in `Compile::Compile`, and also in `Parse::Parse` if that makes us feel safer. Let me know what you think.
> 
> If I remember right, both for OSR and inlining we call the Parser, but not for the intrinsic. We could also try to find a way to set the type of `root` there.

You are right, gvn is initial gvn so you are correct to move it. 
There is an other `Compile()` for runtime stubs.
`PredicatedIntrinsicGenerator::generate()` calls `Parse()` through `ParseGenerator::generate()`
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/callGenerator.cpp#L1239

>> src/hotspot/share/opto/phaseX.cpp line 1245:
>> 
>>> 1243: 
>>> 1244: // Check that if type(n) == n->Value(), return true if we have a failure
>>> 1245: // We have a list of exceptions, see comments in code.
>> 
>> May be enumerate list of exceptions and short description.
>
> Ok, I can do that. I will just make a short list, and a quick sentence per item. The detailed description I will still leave further down, at the block that actually implements the exception, ok?

Yes, like that. And with numbers in short list and in corresponding detailed description.

>> src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp line 317:
>> 
>>> 315:     }
>>> 316:     value = value / 10;
>>> 317:   }
>> 
>> You need to check that `value` is 0 after loop.
>
> @vnkozlov I copied this from `TypeProfileLevel`. Should I also add the 0-check there, and change its type to `uint`?

Yes, please. Both places.  Otherwise it allows to specify values like `876543211` where it should be only allowed `11` max for your flag.

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

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