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: Build and consume FMA and Permute node operands in standard order #103100

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

jakobbotsch
Copy link
Member

The building and consumption of these operands can happen in op1, op2, op3 order regardless of whether the codegen uses the registers in a different order.

Fix #102773

The building and consumption of these operands can happen in op1, op2,
op3 order regardless of whether the codegen uses the registers in a
different order.

Fix dotnet#102773
@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 Jun 5, 2024
Copy link
Contributor

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

{
srcCount += op->isContained() ? BuildOperandUses(op) : BuildDelayFreeUses(op, emitOp1);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This could probably be cleaned up to just build the uses directly without first figuring out emitOp1, emitOp2, emitOp3, but I didn't really understand all the constraints around copiesUpperBits, the preferred target and the contained operand, so I gave up on trying to do it.

@jakobbotsch jakobbotsch marked this pull request as ready for review June 6, 2024 03:00
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @kunalspathak @tannergooding

This should hopefully fix the failures for good this time.

@tannergooding
Copy link
Member

@jakobbotsch there's another case that is doing something similar with operand switching for PermuteVar*x2, I'm guessing it needs an identical fix?

@tannergooding
Copy link
Member

We also, more generally have various commutative nodes that also dynamically pick the tgtPrefUse and containable node via logic in BuildRMWUses. These ones look fine because they look to always build op1 then op2 and codegen (such as genCodeForBinary) then consumes them in the same order via genConsumeOperands even if codegen is going to swap them about

@jakobbotsch
Copy link
Member Author

@jakobbotsch there's another case that is doing something similar with operand switching for PermuteVar*x2, I'm guessing it needs an identical fix?

Ah, good catch, same thing should be applied there. Pushed it to this PR.

We also, more generally have various commutative nodes that also dynamically pick the tgtPrefUse and containable node via logic in BuildRMWUses. These ones look fine because they look to always build op1 then op2 and codegen (such as genCodeForBinary) then consumes them in the same order via genConsumeOperands even if codegen is going to swap them about

Yeah, that looks fine. Seems likely we could try to clean up/unify some of these cases.

@jakobbotsch jakobbotsch changed the title JIT: Build and consume FMA node operands in standard order JIT: Build and consume FMA and Permute node operands in standard order Jun 6, 2024
@jakobbotsch
Copy link
Member Author

No diffs

@jakobbotsch jakobbotsch merged commit f6a7ebb into dotnet:main Jun 6, 2024
114 checks passed
@jakobbotsch jakobbotsch deleted the refix-102773 branch June 6, 2024 22:07
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 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.

SPMI Replay failing sporadically
2 participants