Skip to content

Test failure JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh #61899

Closed
Closed
Test failure JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh#61899
Assignees
kunalspathak
Labels
JitStressCLR JIT issues involving JIT internal stress modesarch-x64area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIos-linuxLinux OS (any supported distro)os-macosmacOS aka OSX
Milestone
7.0.0

Description

@VincentBu
VincentBu
opened on Nov 22, 2021

Run: runtime-coreclr jitstressregs 20211121.1

Failed test:

CoreCLR OSX x64 Checked jitstressregs1 @ OSX.1014.Amd64.Open

- JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh

CoreCLR Linux x64 Checked jitstressregs1 @ Ubuntu.1804.Amd64.Open

- JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh

Error message:

Assert failure(PID 89843 [0x00015ef3], Thread: 487470 [0x7702e]): Assertion failed 'paddingToAdd == paddingNeeded' in 'Benchstone.BenchF.MatInv4:Bench():bool' during 'Emit code' (IL size 321)

File: /Users/runner/work/1/s/src/coreclr/jit/emitxarch.cpp Line: 10070
Image: /private/tmp/helix/working/BC7A09DB/p/corerun
task_for_pid(89843) FAILED 5 (os/kern) failure
/private/tmp/helix/working/BC7A09DB/w/B1520973/e/JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh: line 379: 89843 Abort trap: 6           (core dumped) $LAUNCHER $ExePath "${CLRTestExecutionArguments[@]}"

Return code:      1
Raw output file:      /tmp/helix/working/BC7A09DB/w/B1520973/uploads/Reports/JIT.Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.output.txt
Raw output:
BEGIN EXECUTION
/tmp/helix/working/BC7A09DB/p/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false MatInv4.dll ''
Expected: 100
Actual: 134
END EXECUTION - FAILED
Test Harness Exitcode is : 1
To run the test:

set CORE_ROOT=/tmp/helix/working/BC7A09DB/p
/private/tmp/helix/working/BC7A09DB/w/B1520973/e/JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh
Expected: True
Actual:   False


Stack trace
   at JIT_Performance._CodeQuality_Benchstones_BenchF_MatInv4_MatInv4_MatInv4_._CodeQuality_Benchstones_BenchF_MatInv4_MatInv4_MatInv4_sh()

Activity

  1. VincentBu
    added
    os-linuxLinux OS (any supported distro)
    os-macosmacOS aka OSX
    JitStressCLR JIT issues involving JIT internal stress modes
    arch-x64
    on Nov 22, 2021
  2. ghost added
    area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
    on Nov 22, 2021
  3. ghost added
    untriagedNew issue has not been triaged by the area owner
    on Nov 22, 2021
  5. JulieLeeMSFT
    assigned
    BruceForstall
    on Nov 22, 2021
  6. JulieLeeMSFT
    removed
    untriagedNew issue has not been triaged by the area owner
    on Nov 22, 2021
  7. JulieLeeMSFT
    added this to the 7.0.0 milestone on Nov 22, 2021

  8. JulieLeeMSFT commented on Nov 22, 2021

    @JulieLeeMSFT
    JulieLeeMSFT
    on Nov 22, 2021
    Member

    @BruceForstall PTAL.

  9. BruceForstall
    assigned
    kunalspathak
    and unassigned
    BruceForstall
    on Nov 24, 2021

  10. BruceForstall commented on Nov 24, 2021

    @BruceForstall
    BruceForstall
    on Nov 24, 2021
    Contributor

    @kunalspathak assert related to loop alignment (and also JitStressRegs=1?)

  11. kunalspathak commented on Nov 28, 2021

    @kunalspathak
    kunalspathak
    on Nov 28, 2021
    Contributor

    This is happening precisely because of a scenario for which I added a separate flag to track if alignment flag is removed or not. But after re-thinking from #60787 (comment) I decided to remove it. Here is the scenario:

    We align a loop IG04 that sometimes might contain a jmp instruction that we selected to hide the align instruction for a future loop (IG15 in this case). When we calculate the loop adjustments heuristic, we get the loop size of loop that starts at IG04. We see that IG05 contains align instruction at the end, but since we don't know yet how much padding will be added at the end of IG05, we exclude the 15 bytes padding bytes from the loop size. However, later, when we have to adjust the alignment for the loop that starts at IG15, we might decide to either not add padding at all (because of heuristics) or might add padding <= 15 bytes to it. As such the size of a loop that starts at IG04 will vary compared to the size we calculated our alignment heuristics. As such the presence of a IG that ends with alignment (align behind jmp) is misleading to our calculation and IMO, we should not align such a loop (IG04 in below example).

    ...
IG03:
        <align for IG04>
IG04:                            ; loop start at IG04
       ...
IG05:
      ...
      jmp ...
      <align for IG15>    ; there is another loop at IG15 and we decide to put its alignment in this block.
IG06:
      ...
      jne IG04

...

    Alternatively, one may argue to why bother place align instruction for loop IG15 at the end of IG05 (which is part of a loop) rather than placing it in IG14 which is just before the loop start. We cannot do it currently because during placeLoopAlignInstructions phase, we do not have information of which BasicBlock is part of a loop. I will discuss this offline with @BruceForstall as well before going forward with this approach.

  12. BruceForstall commented on Nov 30, 2021

    @BruceForstall
    BruceForstall
    on Nov 30, 2021
    Contributor

    IIUC, the assert happens because when we go to emit the alignment for IG04, we calculate the alignment padding needed by calling emitCalculatePaddingForLoopAlignment, which involves calculating the loop size of the loop starting at IG04. But if we had previously decided not to align IG15, the align instruction in IG05 would get zeroed (in size), and therefore our original estimate for loop size and final estimate for loop size will differ, which might change our decision about adding padding in IG03 at all?

    Is the assert valid? It seems like we might not be able to guarantee loop size.

    Or, should emitCalculatePaddingForLoopAlignment not include any alignment instruction sizes in the loop size estimate? (It might underestimate actual loop size, which might lead to more loop alignment, but then again, these embedded alignment NOP instructions shouldn't get executed.)

  13. kunalspathak commented on Nov 30, 2021

    @kunalspathak
    kunalspathak
    on Nov 30, 2021
    Contributor

    But if we had previously decided not to align IG15, the align instruction in IG05 would get zeroed (in size),

    If we decide not to align IG15, that is best case scenario because we excluded the padding bytes from our loop size calculation of loop that starts at IG03. However, the problem comes when we decide to align IG15 and hence add padding of XXX bytes. In such case, the IG03 loop size is guarantee to be more by XXX bytes than we originally calculated.

    which might change our decision about adding padding in IG03 at all?

    That's right.

    Is the assert valid? It seems like we might not be able to guarantee loop size.

    The assert is not valid only in two scenarios:

    • If we are adding the padding behind a jmp for some future loop that doesn't immediately follow the current IG. We already account for that:

    runtime/src/coreclr/jit/emitxarch.cpp

    Lines 10037 to 10040 in 85642f8

    // For cases where 'align' was placed behing a 'jmp' in an IG that does not
    // immediately preced the loop IG, we do not know in advance the offset of
    // IG having loop. For such cases, skip the padding calculation validation.
    bool validatePadding = alignInstr->idaIG == alignInstr->idaLoopHeadPredIG;

    I am improving that check further in #62106.

    • For loops that contains a jmp and we decide to hide align behind that jmp which this issue is about. I am proposing to not align such loops and hence we will never be adding padding for such loops.

    Or, should emitCalculatePaddingForLoopAlignment not include any alignment instruction sizes in the loop size estimate?

    We already do that today:

    runtime/src/coreclr/jit/emit.cpp

    Lines 5127 to 5128 in 85642f8

    // The current loop size should exclude the align instruction size reserved for next loop.
    loopSize -= emitComp->opts.compJitAlignPaddingLimit;

  14. kunalspathak commented on Dec 1, 2021

    @kunalspathak
    kunalspathak
    on Dec 1, 2021
    Contributor

    Spoke offline with @BruceForstall and here are few things we concluded:

    • Use bbNaturalLoopNum in placement phase to determine whether to put align in the block that is part of loop
    • Move the loop intersection detection logic from IG-based to BB-based inside placement phase. For this to happen, we might have to do the block renumbering once again before the placement phase (do in follow-up PR).

  15. kunalspathak commented on Dec 1, 2021

    @kunalspathak
    kunalspathak
    on Dec 1, 2021
    Contributor

    I also realized that I was handling the case to add align behind jmp only for IGs that comes after processing the current loop that needs alignment, but it got lost when I added the placement phase.

  16. kunalspathak
    added 2 commits that reference this issue on Dec 2, 2021

    fix for dotnet#61899

    1fc26a5

    Revert "fix for dotnet#61899"

    75a9677

  17. VincentBu commented on Dec 6, 2021

    @VincentBu
    VincentBu
    on Dec 6, 2021
    ContributorAuthor

    Failed again in: runtime-coreclr jitstressregs 20211205.1

    Failed test:

    CoreCLR OSX x64 Checked jitstressregs1 @ OSX.1014.Amd64.Open

- JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh

CoreCLR Linux x64 Checked jitstressregs1 @ Ubuntu.1804.Amd64.Open

- JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh

    Error message:

    Assert failure(PID 77501 [0x00012ebd], Thread: 2661136 [0x289b10]): Assertion failed 'paddingToAdd == paddingNeeded' in 'Benchstone.BenchF.MatInv4:Bench():bool' during 'Emit code' (IL size 321)

File: /Users/runner/work/1/s/src/coreclr/jit/emitxarch.cpp Line: 10065
Image: /private/tmp/helix/working/B7E50A21/p/corerun
task_for_pid(77501) FAILED 5 (os/kern) failure
/private/tmp/helix/working/B7E50A21/w/B4D50974/e/JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh: line 379: 77501 Abort trap: 6           (core dumped) $LAUNCHER $ExePath "${CLRTestExecutionArguments[@]}"

Return code:      1
Raw output file:      /tmp/helix/working/B7E50A21/w/B4D50974/uploads/Reports/JIT.Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.output.txt
Raw output:
BEGIN EXECUTION
/tmp/helix/working/B7E50A21/p/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false MatInv4.dll ''
Expected: 100
Actual: 134
END EXECUTION - FAILED
Test Harness Exitcode is : 1
To run the test:

set CORE_ROOT=/tmp/helix/working/B7E50A21/p
/private/tmp/helix/working/B7E50A21/w/B4D50974/e/JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh
Expected: True
Actual:   False


Stack trace
   at JIT_Performance._CodeQuality_Benchstones_BenchF_MatInv4_MatInv4_MatInv4_._CodeQuality_Benchstones_BenchF_MatInv4_MatInv4_MatInv4_sh()
  18. kunalspathak
    added a commit that references this issue on Dec 6, 2021

    Filter blocks to be used for hiding alignment instruction (#62262)

    a9947a1

  19. kunalspathak commented on Dec 11, 2021

    @kunalspathak
    kunalspathak
    on Dec 11, 2021
    Contributor

    This is fixed in #62262

  20. kunalspathak
    closed this as completedon Dec 11, 2021
  21. ghost locked as resolved and limited conversation to collaborators on Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

JitStressCLR JIT issues involving JIT internal stress modesarch-x64area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIos-linuxLinux OS (any supported distro)os-macosmacOS aka OSX

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions