[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