[lworld] RFR: 8228634: [lworld] ciField::will_link() returns incorrect result for the withfield bytecode

Tobias Hartmann thartmann at openjdk.java.net
Mon Dec 14 07:07:06 UTC 2020

On Fri, 11 Dec 2020 15:13:13 GMT, Frederic Parain <fparain at openjdk.org> wrote:

>> In `GraphBuilder::withfield`, `ciField::will_link()` can return false even if the holder klass is loaded (see bug comments). In that case we should not deoptimize but patch the store instruction that writes the new field value. That is required even if the field offset is known because the field could not be accessible/available and we would need to throw an exception during patching. The load/store instructions emitted by `copy_inline_content` to initialize the fields of the new inline type buffer copy should never require patching: We know their offsets (because the holder is loaded) and we don't need to check access restrictions.
>> Unrelated changes:
>> - Array loads should not be delayed if the subsequent getfield requires patching (see changes to `GraphBuilder::load_indexed`)
>> - Replaced the `WithField` and `DefaultValue` IR nodes by a `Deoptimize` node
>> - Refactoring and removal of dead code
>> Thanks,
>> Tobias
> Hi Tobias,
> With the discussion we had off-line about nestmates, changes look good to me.
> Suggestion for a future improvement: the Deoptimize node in the HIR is a very nice improvement. However, if it looks like a general purpose node in the HIR, the LIRGenerator has a hard coded deopt reason for it: "Reason_unloaded". It would be nice to have a more flexible Deoptmize node where the reason could be specified at the HIR level.
> Thank you,
> Fred

Thanks for the review Fred!

Yes, I agree that the Deoptimize node should be more generic. As we've discussed off-thread, we can address this once more reasons for deoptimization need to be supported (probably with the restricted fields prototype).



PR: https://git.openjdk.java.net/valhalla/pull/301

More information about the valhalla-dev mailing list