On Mon, 21 Oct 2024 04:11:03 GMT, Jasmine Karthikeyan
<[email protected]> wrote:
> Hi all,
> This patch adds a new pass to consolidate lowering of complex
> backend-specific code patterns, such as `MacroLogicV` and the optimization
> proposed by #21244. Moving these optimizations to backend code can simplify
> shared code, while also making it easier to develop more in-depth
> optimizations. The linked bug has an example of a new optimization this could
> enable. The new phase does GVN to de-duplicate nodes and calls nodes'
> `Value()` method, but it does not call `Identity()` or `Ideal()` to avoid
> undoing any changes done during lowering. It also reuses the IGVN worklist to
> avoid needing to re-create the notification mechanism.
>
> In this PR only the skeleton code for the pass is added, moving `MacroLogicV`
> to this system will be done separately in a future patch. Tier 1 tests pass
> on my linux x64 machine. Feedback on this patch would be greatly appreciated!
src/hotspot/cpu/aarch64/c2_lowering_aarch64.cpp line 29:
> 27: #include "opto/phaseX.hpp"
> 28:
> 29: Node* PhaseLowering::lower_node(Node* in) {
You need to wrap all these in `#ifdef COMPILER2`
src/hotspot/share/opto/compile.cpp line 2464:
> 2462: {
> 2463: TracePhase tp("lower", &timers[_t_lower]);
> 2464: print_method(PHASE_BEFORE_LOWERING, 3);
Isn't `BEFORE_LOWERING` the same as `AFTER_BARRIER_EXPANSION` right above?
src/hotspot/share/opto/phaseX.cpp line 2277:
> 2275:
> 2276: // Try to find an existing version of the same node
> 2277: Node* existing = _igvn->hash_find_insert(n);
I think it would be easier if you have a switch in `gvn` that says you passed
the point of doing `Ideal`, moving forward you will probably want to have a
`IdealLowering` to transform nodes during this phase. `Identity` I think is
fine since it returns an existing node.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1808154845
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1808157225
PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1808156752
