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

Vector64 and Vector128.Create doesn't generate movi for long immediates #36254

Closed
tannergooding opened this issue May 11, 2020 · 9 comments · Fixed by #36512
Closed

Vector64 and Vector128.Create doesn't generate movi for long immediates #36254

tannergooding opened this issue May 11, 2020 · 9 comments · Fixed by #36512
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@tannergooding
Copy link
Member

Vector64.Create and Vector128.Create don't generate movi as the other types do for immediate inputs. This appears to be because the logic is using IsCnsIntOrI whioch will miss GT_CNS_LNG: https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/lowerarmarch.cpp#L933-L957

CC. @kunalspathak, @CarolEidt, @echesakovMSFT

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels May 11, 2020
@tannergooding
Copy link
Member Author

I was going to suggest using IsIntegralConst, but It looks like IsIntegralConst it (https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/gentree.h#L7315-L7322) looks to only check GT_CNS_INT on 64-bit, which means we'll miss the case where a GT_CNS_LNG was created on 64-bit (even though the underlying field is size_t and could have been a GT_CNS_INT)

@kunalspathak
Copy link
Member

Thanks for reporting it @tannergooding . Perhaps .Create() is the only API that gets affected by using IsCnsIntOrI.

@kunalspathak kunalspathak self-assigned this May 11, 2020
@tannergooding
Copy link
Member Author

Possibly, but it seems like something that is very easy to hit if you have an API that can take either an int or long.

I'd think that you really don't care if it is an int or long most of the time, you just care what the value is and if it can "fit" in the encoding. For example, a GT_CNS_LNG that is -1 is just as encodable as a GT_CNS_INT of the same value when the immediate is sign-extended.

@echesakov
Copy link
Contributor

@tannergooding Can you give an example when Vector128.Create does not lower to movi?

Regarding your comment

This appears to be because the logic is using IsCnsIntOrI whioch will miss GT_CNS_LNG

I don't think this is a case. GT_CNS_LNG should never appear in the IR on 64-bit target
https://github.com/dotnet/runtime/blob/master/src/coreclr/src/jit/gentree.h#L2920-L2926
since GT_CNS_INT is stored in size_t which is capable to hold both int and long.

@tannergooding
Copy link
Member Author

GT_CNS_LNG should never appear in the IR on 64-bit target

Ah I see, gtNewLconNode just creates a GT_CNS_INT so you should only get one if you manually create a GT_CNS_LNG...

Looks like its missing because we get:

               [000007] -----+------              *  HWINTRINSIC simd16 long DuplicateToVector128
               [000006] -----+------              \--*  CAST      long <- int
               [000005] -----+------                 \--*  CNS_INT   int    31

@AndyAyersMS
Copy link
Member

@kunalspathak milestone? Seems like 5.0, so putting it there for now.

@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label May 13, 2020
@AndyAyersMS AndyAyersMS added this to the 5.0 milestone May 13, 2020
@kunalspathak
Copy link
Member

@tannergooding - Can you confirm if you will be optimizing long as part of #36267?

@tannergooding
Copy link
Member Author

I will not be. Optimizing the casts is a more general issue and one we hit on x86/x64 as well.

@tannergooding
Copy link
Member Author

I can, however, see if it is relatively easy to handle and address it in a follow up PR.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
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 a pull request may close this issue.

5 participants