Skip to content

Conversation

@kunalspathak
Copy link
Contributor

I see that for simd12 node, [000174] (addr node) is marked as contained, but when we generate code for it, we do not update the life of [000030]. Today, we do that only if addr is not contained.

N105 (  3,  2) [000030] ----------z                   t30 =    LCL_VAR   simd12<System.Numerics.Vector3> V01 arg0         u:1 mm5 (last use) REG mm5 $c0
N107 (  3,  3) [000174] -c---------                  t174 =    LCL_ADDR  byref  V16 tmp13        [+0] NA REG NA $200
                                                            /--*  t174   byref  
                                                            +--*  t30    simd12 
N109 ( 10,  8) [000195] nA--G------                         *  STOREIND  simd12 (copy) REG NA $VN.Void

Fixes: #85899

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 9, 2023
@ghost ghost assigned kunalspathak May 9, 2023
@kunalspathak kunalspathak changed the title Update life of Data() node Update life of data node for simd12 May 9, 2023
@ghost
Copy link

ghost commented May 9, 2023

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

Issue Details

I see that for simd12 node, [000174] (addr node) is marked as contained, but when we generate code for it, we do not update the life of [000030]. Today, we do that only if addr is not contained.

N105 (  3,  2) [000030] ----------z                   t30 =    LCL_VAR   simd12<System.Numerics.Vector3> V01 arg0         u:1 mm5 (last use) REG mm5 $c0
N107 (  3,  3) [000174] -c---------                  t174 =    LCL_ADDR  byref  V16 tmp13        [+0] NA REG NA $200
                                                            /--*  t174   byref  
                                                            +--*  t30    simd12 
N109 ( 10,  8) [000195] nA--G------                         *  STOREIND  simd12 (copy) REG NA $VN.Void

Fixes: #85899

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Contributor Author

@SingleAccretion - can you review this to make sure I didn't miss anything?

@SingleAccretion
Copy link
Contributor

I think we should rather move the call to 'genConsumeReg' that's below to before the 'if'.

@kunalspathak
Copy link
Contributor Author

I think we should rather move the call to 'genConsumeReg' that's below to before the 'if'.

I thought about that, but was not sure if that is valid (to consume the reg) given the addr is contained.

@SingleAccretion
Copy link
Contributor

I do not see any issues with that, given that the data itself is not contained.

@kunalspathak
Copy link
Contributor Author

@dotnet/jit-contrib

@tannergooding
Copy link
Member

Not sure this is the "right" fix. Wouldn't the correct change be to update genEmitStoreLclTypeSimd12 to have genConsumeReg(data)?

Otherwise any other callers could also be doing things incorrectly.

@tannergooding
Copy link
Member

If not, then we should probably have a comment in genEmitStoreclTypeSimd12 covering that that the caller is expected to call genConsumeReg, genProduceReg, genUpdateLife, etc

@kunalspathak
Copy link
Contributor Author

Not sure this is the "right" fix. Wouldn't the correct change be to update genEmitStoreLclTypeSimd12 to have genConsumeReg(data)?

Otherwise any other callers could also be doing things incorrectly.

Currently all the liveness updates happens outside of this method and probably e can consider fixing it, but not in this PR.

then we should probably have a comment in

I can do it in my different PR to skip running CI for the comment?

@kunalspathak
Copy link
Contributor Author

I can do it in my different PR to skip running CI for the comment?

#86016

@kunalspathak kunalspathak merged commit 7c1c9d8 into dotnet:main May 9, 2023
@kunalspathak kunalspathak deleted the updatelifeVector3 branch May 9, 2023 21:23
@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure JIT\\SIMD\\Matrix4x4_ro\\Matrix4x4_ro.cmd

4 participants