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

Supported contained bitcast under STORE_LCL_VAR/FLD on arm32/64. #56122

Merged
merged 7 commits into from
Jul 23, 2021

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Jul 22, 2021

Fixes #54841, closes DrewScoggins/performance-2#7131.

coreclr_tests.pmi.Linux.arm64.checked.mch
Total bytes of delta: -36
6 total files with Code Size differences (6 improved, 0 regressed)

libraries_tests.pmi.Linux.arm64.checked.mch
Total bytes of delta: -16
2 total methods with Code Size differences (2 improved, 0 regressed)

coreclr_tests.pmi.windows.arm64.checked.mch
Total bytes of delta: -60
12 total files with Code Size differences (12 improved, 0 regressed)

libraries_tests.pmi.windows.arm64.checked.mch
Total bytes of delta: -16 
2 total files with Code Size differences (2 improved, 0 regressed)

coreclr_tests.pmi.Linux.arm.checked.mch
Total bytes of delta: -12
3 total methods with Code Size differences (3 improved, 0 regressed), 0 unchanged

@sandreenko sandreenko added arch-arm32 arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Jul 22, 2021
@@ -2024,31 +2024,38 @@ instruction CodeGenInterface::ins_Store(var_types dstType, bool aligned /*=false
// Return Value:
// the instruction to use
//
// Notes:
// The function currently does not expect float srcReg with integral dstType and will assert on such cases.
//
instruction CodeGenInterface::ins_StoreFromSrc(regNumber srcReg, var_types dstType, bool aligned /*=false*/)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like it is another function that we should use always instead of ins_Store but currently we don't.
I have updated arm64 to match arm32 and left xarch as it was for now.

@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress, runtime-coreclr outerloop, runtime-coreclr jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@sandreenko
Copy link
Contributor Author

PTAL @kunalspathak @dotnet/jit-contrib ,
the failures are not related (#56174)

break;
#endif // TARGET_64BIT
default:
assert("unexpected write to the stack.");
Copy link
Member

Choose a reason for hiding this comment

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

This assert will never fire (will always evaluate to "true")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed!

Copy link
Member

Choose a reason for hiding this comment

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

We have a few hits of this it seems:

src\coreclr\jit\valuenum.cpp
8811:                        noway_assert("LOCKADD should not appear before lowering");

src\coreclr\inc\clr_std\vector
296:                    assert("push_back: overflow");
314:                        assert("resize: overflow");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noway_assert

yes, I was fixing it in #44132 but we don't have a check for it and add more from time to time.
I guess we can define a deleted "assert(const char*)" and it will give us a compilation failure, won't it?

@kunalspathak
Copy link
Member

You might want to rebase once #56179 is merged.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments.

src/coreclr/jit/instr.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/instr.cpp Outdated Show resolved Hide resolved
@sandreenko sandreenko merged commit 361184f into dotnet:main Jul 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm32 arch-arm64 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.

[Perf] Changes at 6/28/2021 10:33:32 PM support contained bitcast under STORE_LCL_VAR/FLD for armarch.
4 participants