[code-reflection] RFR: Fix SSABraun bug and add SSA tests [v2]

Hannes Greule hgreule at openjdk.org
Wed Sep 10 19:24:25 UTC 2025


On Tue, 9 Sep 2025 18:52:16 GMT, Ruby Chen <duke at openjdk.org> wrote:

>> Fix a bug in SSABraun where, in `tryRemoveTrivialPhi()`, a phi stored in `same` that is later deleted in a recursive `tryRemoveTrivialPhi()` call is still returned despite being deleted. 
>> 
>> Add five tests to TestSSA: `deadCode(), ifelseLoopNested(), violaJones(), binarySearch(),` and `quicksort()`. `violaJones()` is inspired by the method `findFeaturesKernel` in HAT kernel ViolaJones, which is the bug first presented itself.
>
> Ruby Chen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Replace deleted phi after recursive call
>  - Merge branch 'openjdk:code-reflection' into inline
>  - Merge branch 'openjdk:code-reflection' into inline
>  - Merge branch 'openjdk:code-reflection' into inline
>  - Fix SSABraun bug and add SSA tests

Okay I looked into this a bit closer now with your latest change, and I think it makes sense this way. Interestingly, the original implementation works slightly different than what's described in the paper https://github.com/libfirm/libfirm/blob/114012d1d93427e63ba2f2ab51318e8a1b92f06c/ir/ir/ircons.c#L6.
I wonder if this is an oversight in the algorithm described in the paper.

src/jdk.incubator.code/share/classes/jdk/incubator/code/analysis/SSABraun.java line 200:

> 198:         for (Phi user : phiUsers) {
> 199:             Val res = tryRemoveTrivialPhi(user);
> 200:             if (same.equals(user)) {

For consistency, could you also use `==` here?

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

PR Review: https://git.openjdk.org/babylon/pull/542#pullrequestreview-3207523885
PR Review Comment: https://git.openjdk.org/babylon/pull/542#discussion_r2337691489


More information about the babylon-dev mailing list