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

Revert potential UB due to aliasing + more WB removals #111733

Merged
merged 3 commits into from
Jan 26, 2025

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jan 23, 2025

Remove potential UB from #111576 (move it to importer) + enable more cases where we eliminate write barriers, e.g. this:

public ref struct TestS
{
    public MyStruct s1;
    public MyStruct s2;

    public void Test()
    {
        s1 = s2; // no barrier needed, but we generate (two)
    }
}

public struct MyStruct // Not byref-like
{
    public string A;
    public string B;
}

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 23, 2025
Copy link
Contributor

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

@EgorBo
Copy link
Member Author

EgorBo commented Jan 23, 2025

@MihuBot

@EgorBo
Copy link
Member Author

EgorBo commented Jan 23, 2025

Some regressions with barrier-free code e.g. https://www.diffchecker.com/Sz7TI32G/

(when we save a nonbyref-like struct with gc pointers to a field of byreflike struct)

addr = m_store->AsIndir()->Addr();
indirFlags = m_store->gtFlags & GTF_IND_COPYABLE_FLAGS;
indirFlags = m_store->gtFlags & flagsToPropagate;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any benefit to adding something like

if (m_store->AsBlk()->GetLayout()->IsStackOnly())
  indirFlags |= GTF_IND_TGT_NOT_HEAP;

here (and in morphblock.cpp)? Or will m_store already have this flag set for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jakobbotsch I don't see diffs, but this PR already produces too many missing contexts 🙂
Presumably, this is not enough, because some GT_STORE_BLK have this flag without its layout being IsStackOnly. E.g. when we store a non-byreflike struct to byreflike-struct's field

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it makes sense to me to propagate the flags, but it also makes sense to me to set the flags anew here, since the decomposition is "losing" the information at this point. I will leave it up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added. Looks like MihuBot found a few diffs with it

@EgorBo
Copy link
Member Author

EgorBo commented Jan 23, 2025

@MihuBot

@EgorBo
Copy link
Member Author

EgorBo commented Jan 23, 2025

@MihuBot

@EgorBo
Copy link
Member Author

EgorBo commented Jan 23, 2025

PTAL @jakobbotsch Diffs: MihuBot/runtime-utils#932 (SPMI diffs report missing contexts: https://dev.azure.com/dnceng-public/public/_build/results?buildId=926588&view=ms.vss-build-web.run-extensions-tab)

There are a few regressions where WB is lighter than no-WB (e.g. https://www.diffchecker.com/Sz7TI32G/)
And there are some regressions where previous VN based code could detect more opportunities (mostly, mixed with inlining). I managed to create a minimal repro:

[InlineArray(4)]
internal struct ArgumentData<T>
{
    private T _arg0;
}

[StructLayout(LayoutKind.Sequential)]
internal ref struct Test
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    public Test(object a)
    {
        _args[1] = a;
    }

    internal ArgumentData<object?> _args;
}

here we end up with:

               [000011] -ACXG------                         *  STOREIND  ref   
               [000009] --C--------                         +--*  RET_EXPR  byref (for [000007])
               [000010] -----------                         \--*  LCL_VAR   ref    V01 arg1         

and we can't extract class info easily (stind.ref opcode doesn't contain any class token nor we can extract it from ops).

I might look into this and more cases separately (overall it's a regression only for previously merged PR)

@EgorBo EgorBo marked this pull request as ready for review January 23, 2025 14:36
@EgorBo EgorBo requested a review from jakobbotsch January 23, 2025 14:38
@EgorBo EgorBo merged commit 0f0753d into dotnet:main Jan 26, 2025
111 checks passed
@EgorBo EgorBo deleted the revert-aliased-ub branch January 26, 2025 14:47
grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
  Clean up Stopwatch a bit (dotnet#111834)
  JIT: Fix embedded broadcast simd size (dotnet#111638)
  Revert potential UB due to aliasing + more WB removals (dotnet#111733)
  re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
  Handle OSSL 3.4 change to SAN:othername formatting
  JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
  JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
  JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
  [Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
  Change empty subject test certificate to include a critical SAN.
  Fix reversed code offsets in GcInfo (dotnet#111792)
  Swap some libraries areas between leads (dotnet#111816)
  Add left-handed spherical and cylindrical billboards (dotnet#109605)
  JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
  Fix Zip64ExtraField handling (dotnet#111802)
  Add build support for Android+CoreCLR (dotnet#110471)
  arm64: Add bic(s) compact encoding (dotnet#111452)
  JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
  Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
  JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

2 participants