RFR: 8312332: C2: Refactor SWPointer out from SuperWord [v3]

[Pengfei Li]

On Mon, 11 Sep 2023 07:17:17 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Yes, I have found that `pre_loop_end` could be null when we construct `VPointer` in `SuperWord::output()` - see the code in `superword.cpp` (L2574, L2580). It's null because `CountedLoopNode::is_canonical_loop_entry()` returns null at this time. Before my patch, it cannot be null as we cached `_pre_loop_end` in the SuperWord class. To address your concern, my latest commit moves the cache of `_pre_loop_end` from `SuperWord` to `CountedLoopNode`. Some more asserts are also added to make sure it's used for main loops only. (Caching it in `VPointer` doesn't help).
>
> Can you explain a bit more why `CountedLoopNode::is_canonical_loop_entry()` returned `null` at the time, and why you did move the caching?

I think the comment before function `CountedLoopNode::is_canonical_loop_entry()` explains that. I quote it below.

> // Check the shape of the graph at the loop entry. In some cases,
// the shape of the graph does not match the shape outlined below.
// That is caused by the Opaque1 node "protecting" the shape of
// the graph being removed by, for example, the IGVN performed
// in PhaseIdealLoop::build_and_optimize().
//
// After the Opaque1 node has been removed, optimizations (e.g., split-if,
// loop unswitching, and IGVN, or a combination of them) can freely change
// the graph's shape. As a result, the graph shape outlined below cannot
// be guaranteed anymore.
Node* CountedLoopNode::is_canonical_loop_entry() {

Moving the caching is just to address your above concern that we still return `is_not_member` when `pre_loop_end` is null. Before my patch, it cannot be null as we cached `_pre_loop_end` in the SuperWord class. But after decoupling, we cannot access the cache in the SuperWord class anymore.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15013#discussion_r1321580701