Use the register of CAST for contained index operator #74275

kunalspathak · 2022-08-19T19:40:11Z

If index is contained and the underlying operator is GT_CAST we were not handling that case and that led to using REG_COUNT register from index in genScaledAdd() . I have fixed it and also added checks to see if more such cases are missing.

Fixes: #74117

ghost · 2022-08-19T19:40:25Z

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

Issue Details

If index is contained and the underlying operator is GT_CAST we were not handling that case and that led to using REG_COUNT register from index in genScaledAdd() . I have fixed it and also added checks to see if more such cases are missing.

Fixes: #74117

Author:	kunalspathak
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

kunalspathak · 2022-08-22T14:32:24Z

@dotnet/jit-contrib

SingleAccretion · 2022-08-22T16:37:12Z

src/coreclr/jit/codegenarmarch.cpp

+ else if (index->OperIs(GT_CAST))
+ {
+ index = index->AsCast()->gtGetOp1();
+ }


What kind of cast gets here? CAST<long>(int)? But then what is doing the sign/zero-extension?

N006 ( 4, 6) [000018] -c--G----U- t18 = * CAST long <- uint $1c0 /--* t19 byref +--* t18 long N007 ( 6, 8) [000020] ----------- t20 = * LEA(b+(i*1)+0) byref

Hmm, so it is indeed CAST<long>(int). There is still the question of what is doing the extension (is it perhaps unnecessary)?

JulieLeeMSFT · 2022-08-22T21:19:04Z

@kunalspathak, @dotnet/jit-contrib, are we going to backport it to 7?

kunalspathak · 2022-08-22T21:22:34Z

@kunalspathak, @dotnet/jit-contrib, are we going to backport it to 7?

Probably. This issue is around the code that was introduced in .NET 7.

src/coreclr/jit/lower.cpp

SingleAccretion

Code changes LGTM. We may find out the upfront IsSafeToContainMem it too expensive TP-wise and will need to move it down after the other checks, but hopefully not.

I've also filed #74448 to track the complete fix, you could add a link to it from codegen (or not).

Also it would be nice to add a regression test, say:

[MethodImpl(MethodImplOptions.NoInlining)]
private static void Problem(ref byte x, int a)
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static byte GetByte() => 1;

    JitUse(&a);
    Unsafe.Add(ref x, a) = GetByte();
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static void JitUse<T>(T* arg) where T : unmanaged { }

This reverts commit e79d17d92ceb0eed5ae1bfd03c2d1d6b171ab17f.

src/coreclr/jit/lower.cpp

kunalspathak · 2022-08-30T15:06:10Z

@dotnet/jit-contrib - can someone review it?

kunalspathak · 2022-08-31T20:54:49Z

@EgorBo - can you take a look?

src/coreclr/jit/codegenarmarch.cpp

EgorBo · 2022-09-05T14:47:36Z

@EgorBo - can you take a look?

oops, sorry for the delay - didn't notice this mention, something is wrong with my mail filters. LGTM btw

kunalspathak · 2022-09-05T17:12:03Z

/backport to release/7.0-rc1

github-actions · 2022-09-05T17:12:14Z

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2995029976

github-actions · 2022-09-05T17:14:06Z

@kunalspathak backporting to release/7.0-rc1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Rename test files
error: mode change for src/tests/JIT/Regression/JitBlue/Runtime_74373/Runtime_73821.cs, which is not in current HEAD
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Rename test files
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

kunalspathak · 2022-09-06T17:10:33Z

/backport to release/7.0

github-actions · 2022-09-06T17:10:45Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3002058907

github-actions · 2022-09-06T17:12:46Z

@kunalspathak backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Rename test files
Applying: Fix index scale for cast
Applying: Do not contain index if indir is not containable.
Applying: Revert "Do not contain index if indir is not containable."
Applying: Instead try to contain the LEA
Using index info to reconstruct a base tree...
M	src/coreclr/jit/lower.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/lower.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/lower.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Instead try to contain the LEA
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

* Rename test files * Fix index scale for cast * Do not contain index if indir is not containable. * Revert "Do not contain index if indir is not containable." This reverts commit e79d17d92ceb0eed5ae1bfd03c2d1d6b171ab17f. * Instead try to contain the LEA * IsSafeToContainMem() check * Do IsSafeToContainMem() only if needed * Add test case * Fix merge error * fix the test case * review comment

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 19, 2022

ghost assigned kunalspathak Aug 19, 2022

runfoapp bot mentioned this pull request Aug 19, 2022

system.collections.concurrent.tests failed in CI #73038

Closed

kunalspathak force-pushed the index_contained branch from 20b33ba to 090091a Compare August 20, 2022 00:18

kunalspathak mentioned this pull request Aug 20, 2022

Spmi-diff is showing diffs that are not related to the PR #74287

Closed

kunalspathak marked this pull request as ready for review August 22, 2022 14:28

kunalspathak force-pushed the index_contained branch from 090091a to 143c749 Compare August 22, 2022 14:32

SingleAccretion reviewed Aug 22, 2022

View reviewed changes

JulieLeeMSFT added this to the 7.0.0 milestone Aug 22, 2022

SingleAccretion reviewed Aug 23, 2022

View reviewed changes

src/coreclr/jit/lower.cpp Outdated Show resolved Hide resolved

SingleAccretion reviewed Aug 23, 2022

View reviewed changes

src/coreclr/jit/lower.cpp Outdated Show resolved Hide resolved

SingleAccretion mentioned this pull request Aug 23, 2022

Fix and clean up the handling of "must contain" LEAs on ARM64 #74448

Open

SingleAccretion approved these changes Aug 23, 2022

View reviewed changes

kunalspathak added 8 commits August 26, 2022 07:50

Rename test files

7dd7403

Fix index scale for cast

5a06ce6

Do not contain index if indir is not containable.

592fafe

Revert "Do not contain index if indir is not containable."

aae7ba5

This reverts commit e79d17d92ceb0eed5ae1bfd03c2d1d6b171ab17f.

Instead try to contain the LEA

feb4476

IsSafeToContainMem() check

42875bc

Do IsSafeToContainMem() only if needed

2416bda

Add test case

a6d5919

kunalspathak force-pushed the index_contained branch from 37372b3 to a6d5919 Compare August 26, 2022 21:51

SingleAccretion reviewed Aug 26, 2022

View reviewed changes

src/coreclr/jit/lower.cpp Outdated Show resolved Hide resolved

kunalspathak added 3 commits August 26, 2022 15:35

Fix merge error

85c40cf

Merge remote-tracking branch 'origin/main' into index_contained

8e97e94

fix the test case

512e922

BruceForstall approved these changes Sep 2, 2022

View reviewed changes

src/coreclr/jit/codegenarmarch.cpp Outdated Show resolved Hide resolved

review comment

0f14423

kunalspathak merged commit fd11209 into dotnet:main Sep 5, 2022

kunalspathak deleted the index_contained branch September 5, 2022 17:08

carlossanlop mentioned this pull request Sep 6, 2022

[release/7.0] Use the register of CAST for contained index operator #75145

Merged

ghost locked as resolved and limited conversation to collaborators Oct 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the register of CAST for contained index operator #74275

Use the register of CAST for contained index operator #74275

kunalspathak commented Aug 19, 2022

ghost commented Aug 19, 2022

kunalspathak commented Aug 22, 2022

SingleAccretion Aug 22, 2022

kunalspathak Aug 22, 2022

SingleAccretion Aug 22, 2022

JulieLeeMSFT commented Aug 22, 2022

kunalspathak commented Aug 22, 2022

SingleAccretion left a comment •

edited

Loading

kunalspathak commented Aug 30, 2022

kunalspathak commented Aug 31, 2022

EgorBo commented Sep 5, 2022 •

edited

Loading

kunalspathak commented Sep 5, 2022

github-actions bot commented Sep 5, 2022

github-actions bot commented Sep 5, 2022

kunalspathak commented Sep 6, 2022

github-actions bot commented Sep 6, 2022

github-actions bot commented Sep 6, 2022

Use the register of CAST for contained index operator #74275

Use the register of CAST for contained index operator #74275

Conversation

kunalspathak commented Aug 19, 2022

ghost commented Aug 19, 2022

kunalspathak commented Aug 22, 2022

SingleAccretion Aug 22, 2022

Choose a reason for hiding this comment

kunalspathak Aug 22, 2022

Choose a reason for hiding this comment

SingleAccretion Aug 22, 2022

Choose a reason for hiding this comment

JulieLeeMSFT commented Aug 22, 2022

kunalspathak commented Aug 22, 2022

SingleAccretion left a comment • edited Loading

Choose a reason for hiding this comment

kunalspathak commented Aug 30, 2022

kunalspathak commented Aug 31, 2022

EgorBo commented Sep 5, 2022 • edited Loading

kunalspathak commented Sep 5, 2022

github-actions bot commented Sep 5, 2022

github-actions bot commented Sep 5, 2022

kunalspathak commented Sep 6, 2022

github-actions bot commented Sep 6, 2022

github-actions bot commented Sep 6, 2022

SingleAccretion left a comment •

edited

Loading

EgorBo commented Sep 5, 2022 •

edited

Loading