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

Fix parameter ordering and value in gtNewColonNode #87366

Merged
merged 1 commit into from Jun 15, 2023

Conversation

huoyaoyuan
Copy link
Member

To match the constructor parameters of GenTreeColon.

All of the callers are passing arguments in the correct order. The 64 bit BSR implementation of LZCNT gets an int colon instead of long. Since the result is <64, there wasn't incorrect truncation if JIT is tolerate to such type mismatch.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 10, 2023
@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 10, 2023
@ghost
Copy link

ghost commented Jun 10, 2023

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

Issue Details

To match the constructor parameters of GenTreeColon.

All of the callers are passing arguments in the correct order. The 64 bit BSR implementation of LZCNT gets an int colon instead of long. Since the result is <64, there wasn't incorrect truncation if JIT is tolerate to such type mismatch.

Author: huoyaoyuan
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Jun 10, 2023

Does this fix a bug or something? The problem that ElseNode is op1 so it just adds more confusion where we already have it. A proper fix might require more changes

// There was quite a bit of confusion in the code base about which of gtOp1 and gtOp2 was the
// 'then' and 'else' clause of a colon node. Adding these accessors, while not enforcing anything,
// at least *allows* the programmer to be obviously correct.
// However, these conventions seem backward.
// TODO-Cleanup: If we could get these accessors used everywhere, then we could switch them.
struct GenTreeColon : public GenTreeOp
{
GenTree*& ThenNode()
{
return gtOp2;
}
GenTree*& ElseNode()
{
return gtOp1;
}

(see comment)

cc @dotnet/jit-contrib

@huoyaoyuan
Copy link
Member Author

This fixes my confusion when trying to use gtNewColonNode instead of new (this, GT_COLON) GenTreeColon.

GenTreeColon(var_types typ, GenTree* thenNode, GenTree* elseNode) : GenTreeOp(GT_COLON, typ, elseNode, thenNode)
{
}

The parameter names in GenTreeColon constructor are correct, but gtNewColonNode is incorrect. thenNode is correctly stored into op2.

@BruceForstall BruceForstall merged commit af19a79 into dotnet:main Jun 15, 2023
127 checks passed
@huoyaoyuan huoyaoyuan deleted the gtnewcolon branch June 15, 2023 16:35
@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 15, 2023
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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants