Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

JIT: avoid folding operations with relocatable immediates #21511

Merged
merged 2 commits into from
Dec 13, 2018
Merged

JIT: avoid folding operations with relocatable immediates #21511

merged 2 commits into from
Dec 13, 2018

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Dec 12, 2018 •
edited
Loading

In general, don't fold operations on relocatable immediates. Only allow EQ/NE as relocation should preserve identity but not bit values or relative comparisons.

Closes #21483.

@AndyAyersMS
JIT: broaden set of operators for foldable relocatable immediates
ccd882e 
Allow full range of compare ops, not just EQ and NE.

Closes #21483.
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Dec 12, 2018 •
edited
Loading

@BruceForstall PTAL
cc @dotnet/jit-contrib

Just relaxes asserts, so no diffs expected.

Avoid folding except for EQ/NE. Still no diffs expected (as we'd assert before).

briansull
briansull suggested changes Dec 12, 2018
View reviewed changes
src/jit/gentree.cpp Outdated
@@ -15901,7 +15901,7 @@ bool GenTreeIntConCommon::ImmedValCanBeFolded(Compiler* comp, genTreeOps op)
// In general, immediate values that need relocations can't be folded.
// There are cases where we do want to allow folding of handle comparisons
// (e.g., typeof(T) == typeof(int)).
return !ImmedValNeedsReloc(comp) || (op == GT_EQ) || (op == GT_NE);
Copy link

Choose a reason for hiding this comment

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

The guard is here because when relocs are involved you can not order the immediates
(i..e. you can't know at compile time if one reloc-ed value is greater or less than another)

Copy link

Choose a reason for hiding this comment

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

As a special case when both the left and right sides are exactly the same reloc then we could fold the other relops (GE, GT, LE and LT), since we know they are Equal

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps then just we should bail out if we see non-foldable immediates instead of asserting, and not worry about the special cases.

briansull
briansull approved these changes Dec 12, 2018
View reviewed changes
Copy link

@briansull briansull left a comment

Choose a reason for hiding this comment

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

Looks Good

@briansull
Copy link

You should rename this PR from:

JIT: broaden set of operators for foldable relocatable immediates

@AndyAyersMS AndyAyersMS changed the title JIT: broaden set of operators for foldable relocatable immediates JIT: avoid folding operations with relocatable immediates Dec 13, 2018
@AndyAyersMS
Copy link
Member Author

Can't see the ubuntu failure logs, so will rerun...

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test

adding some r2r legs:

@dotnet-bot test Ubuntu x64 Checked r2r_no_tiered_compilation
@dotnet-bot test CentOS7.1 x64 Checked r2r

@AndyAyersMS
Copy link
Member Author

Looks like ci got rebooted, so need to request again:

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test

@dotnet-bot test Ubuntu x64 Checked r2r_no_tiered_compilation
@dotnet-bot test CentOS7.1 x64 Checked r2r

@AndyAyersMS
Copy link
Member Author

More CI issues...

OSX and the Ubuntu arm tests all have msbuild crashes while building tests:

13:43:14   nesting3 -> /Users/dotnet-bot/j/workspace/dotnet_coreclr/master/x64_checked_osx10.12_innerloop_prtest/bin/tests/OSX.x64.Checked/Loader/classloader/nesting/coreclr/nesting3/nesting3.dll
13:43:15 MSBUILD : error MSB4166: Child node "4" exited prematurely. Shutting down. Diagnostic information may be found in files in "/tmp/" and will be named MSBuild_*.failure.txt. This location can be changed by setting the MSBUILDDEBUGPATH environment variable to a different directory.
13:43:16 
13:43:16 Build FAILED.

@AndyAyersMS
Copy link
Member Author

Various R2R test legs passed. Am going to ignore the other failures.

@AndyAyersMS AndyAyersMS merged commit 6b9e1dc into dotnet:master Dec 13, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@AndyAyersMS
JIT: avoid folding operations with relocatable immediates (dotnet/cor…
cdd65fb 
…eclr#21511)

In general, don't fold operations on relocatable immediates. Only allow EQ/NE folding, since relocation should preserve identity but not bit values or relative comparisons.

Closes dotnet/coreclr#21483.

Commit migrated from dotnet/coreclr@6b9e1dc
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BruceForstall BruceForstall BruceForstall approved these changes

@briansull briansull briansull approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@AndyAyersMS @briansull @BruceForstall