Skip to content

[cDAC] Draft CodeVersionsTest refactor#2

Closed
max-charlamb wants to merge 1 commit intomainfrom
cdac-update-cv-tests
Closed

[cDAC] Draft CodeVersionsTest refactor#2
max-charlamb wants to merge 1 commit intomainfrom
cdac-update-cv-tests

Conversation

@max-charlamb
Copy link
Copy Markdown
Owner

@max-charlamb max-charlamb commented Dec 13, 2024

No description provided.

max-charlamb
max-charlamb commented Dec 13, 2024
View reviewed changes
Comment thread src/native/managed/cdacreader/tests/CodeVersionsTests.cs Outdated
private IReadOnlyCollection<MockCodeBlockStart> _codeBlocks;
private Mock<IRuntimeTypeSystem> mockRuntimeTypeSystem = new(MockBehavior.Strict);
private Mock<IExecutionManager> mockExecutionManager = new(MockBehavior.Strict);
private Mock<ILoader> mockLoader = new(MockBehavior.Strict);
Copy link
Copy Markdown
Owner Author

@max-charlamb max-charlamb Dec 13, 2024

Choose a reason for hiding this comment

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

MockBehavior.Strict isn't required, but I wanted to make sure I was capturing all the calls.

max-charlamb
max-charlamb commented Dec 13, 2024
View reviewed changes
Comment thread src/native/managed/cdacreader/tests/CodeVersionsTests.cs Outdated
ulong addressLowBits = (ulong)address & ((ulong)_target.PointerSize - 1);

// this is not quite accurate on 32 bit architectures, but it's good enough for testing
ulong addressLowBits = (ulong)address & 7;
Copy link
Copy Markdown
Owner Author

@max-charlamb max-charlamb Dec 13, 2024

Choose a reason for hiding this comment

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

Should be (ulong)address & ((ulong)_target.PointerSize - 1);, however when we set this up the target isn't created. I guess we could create the target first and pass it in.

Copy link
Copy Markdown

@elinor-fung elinor-fung Dec 13, 2024

Choose a reason for hiding this comment

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

You should be able to get the pointer size off of the TargetTestHelpers for MockDescriptor.CodeVersions, which we create before doing this.

max-charlamb
max-charlamb commented Dec 13, 2024
View reviewed changes
Comment thread src/native/managed/cdacreader/tests/CodeVersionsTests.cs
[ClassData(typeof(MockTarget.StdArch))]
public void GetNativeCodeVersion_Null(MockTarget.Architecture arch)
{
mockExecutionManager.Setup(e => e.GetCodeBlockHandle(TargetCodePointer.Null)).Returns(() => null);
Copy link
Copy Markdown
Owner Author

@max-charlamb max-charlamb Dec 13, 2024

Choose a reason for hiding this comment

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

Only required with MockBehavior.Strict. Otherwise this test relies on the mock returning null when there is no setup. I find this a bit confusing.

elinor-fung
elinor-fung reviewed Dec 14, 2024
View reviewed changes
Comment thread src/native/managed/cdacreader/tests/CodeVersionsTests.cs Outdated

TargetCodePointer IExecutionManager.GetStartAddress(CodeBlockHandle codeInfoHandle) => FindCodeBlock(codeInfoHandle.Address)?.StartAddress ?? TargetCodePointer.Null;
TargetPointer IExecutionManager.GetMethodDesc(CodeBlockHandle codeInfoHandle) => FindCodeBlock(codeInfoHandle.Address)?.MethodDescAddress ?? TargetPointer.Null;
private void AddCodeBlock(MockCodeBlockStart block)
Copy link
Copy Markdown

@elinor-fung elinor-fung Dec 14, 2024

Choose a reason for hiding this comment

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

Do you think MockCodeBlockStart and friends are still helpful? Versus having these method just take in the address, length, method desc, etc. as separate parameters.

Copy link
Copy Markdown
Owner Author

@max-charlamb max-charlamb Dec 16, 2024

Choose a reason for hiding this comment

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

I think they are still helpful to encapsulate state related to a mock. I think if we removed them, it would make tests with multiple MethodDescs confusing due to keeping track of multiple similar fields.

elinor-fung
elinor-fung reviewed Dec 14, 2024
View reviewed changes
Comment thread src/native/managed/cdacreader/tests/CodeVersionsTests.cs Outdated
internal class MockExecutionManager : IExecutionManager
{
private IReadOnlyCollection<MockCodeBlockStart> _codeBlocks;
private Mock<IRuntimeTypeSystem> mockRuntimeTypeSystem = new(MockBehavior.Strict);
Copy link
Copy Markdown

@elinor-fung elinor-fung Dec 14, 2024

Choose a reason for hiding this comment

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

I tend to find it easier to reason about tests when everything is contained in the test method (explicitly sets up what it needs / no state outside the method, don't have to know that each test method actually gets a new test class instance).

Maybe try something like having the test cases explicitly create the mock(s) they need and turn the Add* methods into extension methods for Mock<...>?

Copy link
Copy Markdown
Owner Author

@max-charlamb max-charlamb Dec 16, 2024

Choose a reason for hiding this comment

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

Happy to make this change. I settled on having a MockExtensions class which adds the helpers as extensions on the correct generic implementation of Mock<...>.

@max-charlamb max-charlamb changed the base branch from md-gccover to main December 16, 2024 17:48
@max-charlamb max-charlamb changed the base branch from main to md-dynamic December 16, 2024 17:49
@max-charlamb max-charlamb changed the base branch from md-dynamic to main December 16, 2024 17:49
@max-charlamb max-charlamb mentioned this pull request Dec 16, 2024
Merged
@max-charlamb max-charlamb deleted the cdac-update-cv-tests branch June 10, 2025 16:37
max-charlamb pushed a commit that referenced this pull request Mar 24, 2026
@yykkibbb
JIT: Fix fgFoldCondToReturnBlock for multi-statement return blocks (d…
7c58a24 
…otnet#124642)

## Summary

Fixes dotnet#123621

When a constant-folded operand appears **after** a non-constant operand
in a short-circuit `&&` expression (e.g., `v == 2 && Environment.NewLine
!= "\r\n"`), callee inlining can leave dead local stores in the return
block. The `isReturnBool` lambda in `fgFoldCondToReturnBlock` required
`hasSingleStmt()`, which caused the optimization to bail out when these
dead stores were present, resulting in suboptimal branching codegen.

### Changes

- **`src/coreclr/jit/optimizebools.cpp`**: Relax the `hasSingleStmt()`
constraint in `isReturnBool` to allow preceding statements as long as
they have no globally visible side effects
(`GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS`). This enables
`fgFoldCondToReturnBlock` to fold the conditional into a branchless
return even when dead local stores from inlining remain in the block.

### Before (ARM64, `Inline_After`)
```asm
      cmp     w0, #2
      bne     G_M4495_IG04
      mov     w0, #1
      ret
G_M4495_IG04:
      mov     w0, #0
      ret
```

### After (ARM64, `Inline_After`)
```asm
      cmp     w0, #2
      cset    x0, eq
      ret
```

## Test plan

- [x] Added regression test `Runtime_123621` covering the original issue
pattern
- [x] Verified `Hoisted`, `Inline_Before`, and `Inline_After` all
produce identical branchless codegen (`cset` on ARM64)
- [x] Verified existing `DevDiv_168744` regression test still passes
- [x] Verified side-effect-ful blocks are correctly excluded from the
optimization
max-charlamb pushed a commit that referenced this pull request Apr 22, 2026
Address remaining PR review comments
1a92e2c 
- Fix README test filter syntax: use FullyQualifiedName~BasicAlloc (#1)
- Remove goto statements from GCInfoDecoder.EnumerateLiveSlots: extract
  ReportUntrackedAndSucceed local function (#2)
- Move CheckForSkippedFrames from Next() to UpdateState (#6)
- Add XUnitConsoleRunner package reference for Helix payload (#9)
- Support TypeSpec (tag=2) in DecodeTypeDefOrRefOrSpec matching native
  CorSigUncompressToken behavior (#10)
- Fix IsAppleArm64ABI: set to false until Apple platform detection is
  available (filed dotnet#127282) (#11)
- Fix Unix x64 float register stride: use FloatRegisterSize instead of
  hardcoded 8 (#12)
- Replace FrameIterator.OffsetFromGCRefMapPos with CallingConventionInfo
  version that handles x86 reversed register layout (dotnet#13)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
max-charlamb pushed a commit that referenced this pull request Apr 22, 2026
Address remaining PR review comments
b85dbac 
- Fix README test filter syntax: use FullyQualifiedName~BasicAlloc (#1)
- Remove goto statements from GCInfoDecoder.EnumerateLiveSlots: extract
  ReportUntrackedAndSucceed local function (#2)
- Move CheckForSkippedFrames from Next() to UpdateState (#6)
- Add XUnitConsoleRunner package reference for Helix payload (#9)
- Support TypeSpec (tag=2) in DecodeTypeDefOrRefOrSpec matching native
  CorSigUncompressToken behavior (#10)
- Fix IsAppleArm64ABI: set to false until Apple platform detection is
  available (filed dotnet#127282) (#11)
- Fix Unix x64 float register stride: use FloatRegisterSize instead of
  hardcoded 8 (#12)
- Replace FrameIterator.OffsetFromGCRefMapPos with CallingConventionInfo
  version that handles x86 reversed register layout (dotnet#13)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
max-charlamb pushed a commit that referenced this pull request Apr 23, 2026
Address remaining PR review comments
8eae621 
- Fix README test filter syntax: use FullyQualifiedName~BasicAlloc (#1)
- Remove goto statements from GCInfoDecoder.EnumerateLiveSlots: extract
  ReportUntrackedAndSucceed local function (#2)
- Move CheckForSkippedFrames from Next() to UpdateState (#6)
- Add XUnitConsoleRunner package reference for Helix payload (#9)
- Support TypeSpec (tag=2) in DecodeTypeDefOrRefOrSpec matching native
  CorSigUncompressToken behavior (#10)
- Fix IsAppleArm64ABI: set to false until Apple platform detection is
  available (filed dotnet#127282) (#11)
- Fix Unix x64 float register stride: use FloatRegisterSize instead of
  hardcoded 8 (#12)
- Replace FrameIterator.OffsetFromGCRefMapPos with CallingConventionInfo
  version that handles x86 reversed register layout (dotnet#13)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
max-charlamb pushed a commit that referenced this pull request Apr 23, 2026
Address remaining PR review comments
49c8435 
- Fix README test filter syntax: use FullyQualifiedName~BasicAlloc (#1)
- Remove goto statements from GCInfoDecoder.EnumerateLiveSlots: extract
  ReportUntrackedAndSucceed local function (#2)
- Move CheckForSkippedFrames from Next() to UpdateState (#6)
- Add XUnitConsoleRunner package reference for Helix payload (#9)
- Support TypeSpec (tag=2) in DecodeTypeDefOrRefOrSpec matching native
  CorSigUncompressToken behavior (#10)
- Fix IsAppleArm64ABI: set to false until Apple platform detection is
  available (filed dotnet#127282) (#11)
- Fix Unix x64 float register stride: use FloatRegisterSize instead of
  hardcoded 8 (#12)
- Replace FrameIterator.OffsetFromGCRefMapPos with CallingConventionInfo
  version that handles x86 reversed register layout (dotnet#13)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
max-charlamb pushed a commit that referenced this pull request Apr 24, 2026
Address remaining PR review comments
de5cb46 
- Fix README test filter syntax: use FullyQualifiedName~BasicAlloc (#1)
- Remove goto statements from GCInfoDecoder.EnumerateLiveSlots: extract
  ReportUntrackedAndSucceed local function (#2)
- Move CheckForSkippedFrames from Next() to UpdateState (#6)
- Add XUnitConsoleRunner package reference for Helix payload (#9)
- Support TypeSpec (tag=2) in DecodeTypeDefOrRefOrSpec matching native
  CorSigUncompressToken behavior (#10)
- Fix IsAppleArm64ABI: set to false until Apple platform detection is
  available (filed dotnet#127282) (#11)
- Fix Unix x64 float register stride: use FloatRegisterSize instead of
  hardcoded 8 (#12)
- Replace FrameIterator.OffsetFromGCRefMapPos with CallingConventionInfo
  version that handles x86 reversed register layout (dotnet#13)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@elinor-fung elinor-fung elinor-fung left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@max-charlamb @elinor-fung