On Thu, 24 Oct 2024 02:14:46 GMT, Jasmine Karthikeyan <jkarthike...@openjdk.org> wrote:
>> Build changes look good (but would be slightly better without the extra >> blank line). I have not reviewed the actual hotspot changes. > > Thanks for looking at the build changes, @magicus! I've pushed a commit that > removes the extra newline in the makefiles and adds newlines to the ends of > files that were missing them. > > Thanks for taking a look as well, @merykitty and @jatin-bhateja! I've pushed > a commit that should address the code suggestions and left some comments as > well. > @jaskarth thanks for exploring platform-specific lowering! > > I briefly looked through the changes, but I didn't get a good understanding > of its design goals. It's hard to see what use cases it is targeted for when > only skeleton code is present. It would really help if there are some cases > ported on top of it for illustration purposes and to solidify the design. > > Currently, there are multiple places in the code where IR lowering happens. > In particular: > > * IGVN during post loop opts phase (guarded by > `Compile::post_loop_opts_phase()`) (Ideal -> Ideal); > * macro expansion (Ideal -> Ideal); > * ad-hoc passes (GC barrier expansion, `MacroLogicV`) (Ideal -> Ideal); > * final graph reshaping (Ideal -> Ideal); > * matcher (Ideal -> Mach). > > I'd like to understand how the new pass is intended to interact with existing > cases. > > Only the last one is truly platform-specific, but there are some > platform-specific cases exposes in other places (e.g., MacroLogicV pass, > DivMod combining) guarded by some predicates on `Matcher`. > > As the `PhaseLowering` is implemented now, it looks like a platform-specific > macro expansion pass (1->many rewriting). But `MacroLogicV` case doesn't fit > such model well. > > I see changes to enable platform-specific node classes. As of now, only > Matcher introduces platform-specific nodes and all of them are Mach nodes. > Platform-specific Ideal nodes are declared in shared code, but then their > usages are guarded by `Matcher::has_match_rule()` thus ensuring there's > enough support on back-end side. > > Some random observations: > > * the pass is performed unconditionally and it iterates over all live nodes; > in contrast, macro nodes and nodes for post-loop opts IGVN are explicitly > listed on the side (MacroLogicV pass is also guarded, but by a coarse-grained > check); MacroLogicV currently is a standalone pass and was not implemented as discrete Idealizations split across various logic IR Idealizations, our intention was to create a standalone pass and limit the exposure of such complex logic packing to rest of the code and guard it by target specific match_rule_supported check. To fit this into new lowering interface, we may need to split root detection across various logic IR node accepted by PhaseLowering::lower_to(Node*). @jaskarth, @merykitty , can you share more use cases apart from VPMUL[U]DQ and MacroLogicV detection in support of new lowring pass. Please be aware that even after introducing lowering pass we still want to prevent replcating IR transforms, e.g transformations/Idealizations only applicable to x86 and AARCH64 should still be shared for maintainability reasons. In general compilers having lowering pass also introduce target specific pre matching IR. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21599#issuecomment-2439800311
