Skip to content

Conversation

@sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Jul 11, 2020

Fixes #39128 and adds IL test back (the issue was in using i8 instead of inative on x86).

I want to keep the transformation enabled for minopts, I think it is not dangerous after this fix and could give us better TP (logic for storeind is smaller in codegen), but I have not measured.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 11, 2020
@sandreenko
Copy link
Contributor Author

PTAL @dotnet/jit-contrib

@AndyAyersMS
Copy link
Member

Would strongly recommend not enabling this for minopts, so we have a way users can work around this if issues arise. If you think it is beneficial for Tier0, we can consider enabling as part of #9120.

Also would like to see some more widespread testing of WPF.

@sandreenko
Copy link
Contributor Author

Would strongly recommend not enabling this for minopts, so we have a way users can work around this if issues arise. If you think it is beneficial for Tier0, we can consider enabling as part of #9120.

Ok, I will disable this for minopts. However, we have only one check for OptimizationEnabled in lower* files and all other optimization transformations are happening in in minopts, so I do not see why this should be different.

Also would like to see some more widespread testing of WPF.

In general or in this PR? I have checked other scenarios similar to what was causing the issue, like copying a primitive and copying size that doesn't match field/struct sizes, but they did not show any new issues.

@AndyAyersMS
Copy link
Member

optimization transformations are happening in in minopts

This is a bad precedent. We really should back off on those optimizations as well. If we think they're generally safe and TP improvements we can enable them in Tier0 too.

Also would like to see some more widespread testing of WPF.

In general or in this PR? I have checked other scenarios similar to what was causing the issue, like copying a primitive and copying size that doesn't match field/struct sizes, but they did not show any new issues.

I'm mostly concerned about properly handling the variety of pinvokes and struct marshalls that run as part as WPF, and also more thorough testing of C++/CLI created IL that also is used there.

I'd prefer it be part of this PR. Since this will be manual testing, we should also write up how to do it in case we want to run it again.

@sandreenko
Copy link
Contributor Author

This is a bad precedent. We really should back off on those optimizations as well. If we think they're generally safe and TP improvements we can enable them in Tier0 too.

I have added the check for TryTransformStoreObjAsStoreInd.

I'd prefer it be part of this PR. Since this will be manual testing, we should also write up how to do it in case we want to run it again.

Unfortunately, it did not happen. Still I think it is safer to get this change in Prev8 and get testing from WPF there than in RC1.

Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

The changes LGTM, and I would agree with @AndyAyersMS that the transformation should be disabled for minopts.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

I have pinged the WPF contributors and haven't heard back. Let's keep trying to work up some plan for doing better WPF testing.

cc @mangod9

@sandreenko sandreenko merged commit 3a4a49b into dotnet:master Jul 15, 2020
@sandreenko sandreenko deleted the fixSTORE_IND branch July 15, 2020 17:26
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 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 this pull request may close these issues.

Fix ASG struct (copy) with different src and dst sizes after global morph.

4 participants