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

[mono] Avoid transformation from multiplication to left shift in case of 64 bit value #71189

Merged
merged 1 commit into from
Jul 25, 2022

Conversation

alhad-deshpande
Copy link
Contributor

This has fixed following issues:

  1. Build failure for ppc64le
  2. Avoid transformation from multiplication to left shift in case of 64 bit value. This has fixed several test failures which invokes Double.TryFormat function including Microsoft.CSharp.Tests in debug build (observed and tested on ppc64le).

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 23, 2022
@dnfadmin
Copy link

dnfadmin commented Jun 23, 2022

CLA assistant check
All CLA requirements met.

@@ -350,7 +350,7 @@ mono_strength_reduction_ins (MonoCompile *cfg, MonoInst *ins, const char **spec)
ins->opcode = OP_INEG;
} else if ((ins->opcode == OP_LMUL_IMM) && (ins->inst_imm == -1)) {
ins->opcode = OP_LNEG;
} else if (ins->inst_imm > 0) {
} else if (ins->inst_imm > 0 && ins->inst_imm <= UINT32_MAX) {
int power2 = mono_is_power_of_two (GTMREG_TO_UINT32 (ins->inst_imm));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't this fail on other platforms ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only fails in debug mode. Not sure if other platforms are checking in debug mode or not. But power arch has 8 byte registers and many test cases fails due to this wherever Double.TryFormat method is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

This works on every other platform, it would better to figure out why it fails on this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vargaz
After figuring out a lot on power, identified that the problem is multiplication instruction is getting changed to left shift instruction due to the data loss which is happening for one of the 64 bit values as mono_is_power_of_two accepts uint32 and not uint64. Hence added a safety check here to avoid the data loss. There is no other place where this issue can be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vargaz
I have tested the multiplication on x64 and power. The program works fine on x64 and does not work on power Below are the console and generated IL codes for both plaforms along with the small test program. The only thing which I can think of is, this is too hardware specific and while passing '0x0000271000000001' int64 parameter as int32 parameter to mono_is_power_of_two function, power is somehow making it to power of 2 which is not happening on x64.

Program:
public static void Main()
{
const ulong mul = 0x0000271000000001; // 1 + (10000ULL << 32)
ulong val = 179769;
ulong result = val * mul;
Console.WriteLine("val = {0}, mul = {1}, result = {2}", val, mul, result);
}

x64 console log:
root@dotnet-node3:/dotnet-sdk-7.0.100-preview.5.22307.18-linux-x64/TestMultiplication/bin/Debug/net7.0# mono TestMultiplication.exe
val = 179769, mul = 42949672960001, result = 7721019758346419769
ppc64le console log:
root@dotnet:
/dotnet-ppc64le/runtime/artifacts/bin/TestMultiplication/Debug/net7.0# mono TestMultiplication.exe
val = 179769, mul = 42949672960001, result = 179769
Below are the IL code, on ppc64le multiplication instruction gets converted to left shit.
x64 IL code:
DTREE TestProgram.Program:Main () 0
BB0(dfn=0) (IDOM=BB-1): BB0
BB3(dfn=1) (IDOM=BB0): BB0 BB3
BB1(dfn=2) (IDOM=BB3): BB0 BB3 BB1
INSERTING SAFEPOINTS
BEFORE SAFEPOINTS 0: [IN: , OUT: BB3(1) ]
BEFORE SAFEPOINTS 3: [IN: BB0(0), OUT: BB1(2) ]
il_seq_point il: 0x0
il_seq_point il: 0x1
il_seq_point il: 0x8
i8const R20 <- [179769]
i8const R21 <- [42949672960001]
long_mul R17 <- R20 R21 clobbers: 1
ppc64le IL code:
DTREE TestProgram.Program:Main () 0
BB0(dfn=0) (IDOM=BB-1): BB0
BB2(dfn=1) (IDOM=BB0): BB0 BB2
BB1(dfn=2) (IDOM=BB2): BB0 BB2 BB1
INSERTING SAFEPOINTS
BEFORE SAFEPOINTS 0: [IN: , OUT: BB2(1) ]
BEFORE SAFEPOINTS 2: [IN: BB0(0), OUT: BB1(2) ]
il_seq_point intr il: 0x0
il_seq_point il: 0x1
il_seq_point il: 0x8
i8const R36 <- [179769]
long_shl_imm R33 <- R36

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vargaz
I have debugged this issue on x64 and power and problem is that mono strength reduction is only happening on power for multiplication. This is because of code paths are getting differentiated at some point. I am trying to find where the code paths are getting differentiated and also trying to find a new power specific fix.

@vargaz
Copy link
Contributor

vargaz commented Jul 24, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz vargaz merged commit 02ffbcd into dotnet:main Jul 25, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Codegen-JIT-mono community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants