Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Change VN's representation for phi definitions #105198

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

jakobbotsch
Copy link
Member

Replace VNF_PhiDef and VNF_MemoryPhiDef by new explicit representations that represent all phi args directly.

Replace `VNF_PhiDef` and `VNF_MemoryPhiDef` by new explicit
representations that represent all phi args directly.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 21, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@jakobbotsch
Copy link
Member Author

A few diffs I want to look at the cause of. I probably won't get to that until after the snap.

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Jul 23, 2024

The diffs look related to ordering in VNs. In the baseline we have:

N001 [000862]   CNS_INT(h) 0x400000000042cea8 static => $208 {Hnd const: 0x400000000042CEA8 GTF_ICON_STATIC_HDL}
N002 [000863]   CNS_INT   0 Fseq[<unknown field>] => $242 {LngCns 0}
N003 [000864]   ADD       => $380 {ADD($208, $242)}

where we didn't fold this handle + constant. The handle has the lower raw VN here. In the diff we have

N001 [000862]   CNS_INT(h) 0x400000000042cea8 static => $282 {Hnd const: 0x400000000042CEA8 GTF_ICON_STATIC_HDL}
N002 [000863]   CNS_INT   0 Fseq[<unknown field>] => $202 {LngCns 0}
N003 [000864]   ADD       => $282 {Hnd const: 0x400000000042CEA8 GTF_ICON_STATIC_HDL}

where we did fold it. The handle has the higher raw VN here. We ended up with the operands to VNF_ADD in the other order and then it seems like EvalUsingMathIdentity kicked in in the diff.

@jakobbotsch jakobbotsch marked this pull request as ready for review July 23, 2024 11:09
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @EgorBo

Diffs. Minor diffs due to the above. Some TP improvements otherwise.
Code wise it looks like more code, but it's mainly boilerplate from adding the two new CEA_PhiDef and CEA_MemoryPhiDef. The actual logic that reasons about phi defs/memory phi defs definitely looks simpler now.

@jakobbotsch jakobbotsch requested a review from EgorBo July 23, 2024 11:11
BasicBlock* Block;
unsigned* SsaArgs;
unsigned NumArgs;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably use a single struct and unify memory with regular phi. It seems like we often probe both anyway, but LGTM as is too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it would require introducing some "kind" type in the unified struct anyway that you will need to check. To actually use the SSA args you need to differentiate between whether it's memory or local, so I'm not sure it simplifies much. I think I'll leave it like this and then we can introduce a helper if there is a good way to unify it.

@jakobbotsch jakobbotsch merged commit 7d86f88 into dotnet:main Jul 23, 2024
103 of 107 checks passed
@jakobbotsch jakobbotsch deleted the vn-phi-repr branch July 23, 2024 14:25
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants