On Thu, 24 Oct 2024 02:14:46 GMT, Jasmine Karthikeyan
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