On Fri, 25 Oct 2024 23:30:52 GMT, Vladimir Ivanov <vliva...@openjdk.org> wrote:
>> 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); @iwanowww The pass is supposed to be a generalisation of the `MacroLogicV` pass, it should be able to perform arbitrary transformations and its immediate usages are for `MacroLogicV` patterns and `vpmuludq` patterns. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21599#issuecomment-2439189524
