Skip to content

NativeAOT/win/arm64: Fix the reloc type typo - #104516

Merged
kunalspathak merged 13 commits into
dotnet:mainfrom
kunalspathak:aot-diffs
Jul 20, 2024
Merged

NativeAOT/win/arm64: Fix the reloc type typo#104516
kunalspathak merged 13 commits into
dotnet:mainfrom
kunalspathak:aot-diffs

Conversation

@kunalspathak

@kunalspathak kunalspathak commented Jul 7, 2024

Copy link
Copy Markdown
Contributor
  • There was a typo, where a relocation should be IMAGE_REL_ARM64_SECREL_LOW12A instead of IMAGE_REL_ARM64_SECREL_LOW12L .
  • The codegen was not handling the add operation correctly, so fixed that.
  • Fixed the handling of the new relocs in superpmi.
  • Added Arm64 target in reproNative.vcxproj for developing/testing on arm64.

Fixes: #104500

@kunalspathak

Copy link
Copy Markdown
Contributor Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).
@VSadov

VSadov commented Jul 8, 2024

Copy link
Copy Markdown
Member

I looked at your sample of the emitted native code and I think there is also an issue with _tls_index temporarily tracked as GC pointer.
here we are indirectly loading an int32 from _tls_index into w1

adrp  x1, [HIGH RELOC _tls_index]
add   x1, x1, [LOW RELOC _tls_index]
   ; gcrRegs +[x1]			<-- this seems incorrect.  _tls_index is a native datastructure
ldr   w1, [x1]				
   ; gcrRegs -[x1]

x1 should be just native int, not a gc pointer.

also the asm/msvc versions skip the add entirely.
ideally this should be

adrp  x1, [HIGH RELOC _tls_index]
ldr   w1, [x1, LOW RELOC _tls_index]				
@VSadov

VSadov commented Jul 8, 2024

Copy link
Copy Markdown
Member

The whole thing could be

        adrp        $destReg, [HIGH RELOC _tls_index]
        ldr         $TrashRegister32Bit, [$destReg, LOW RELOC _tls_index]

        ldr         $destReg, [xpr, #__tls_array]
        ldr         $destReg, [$destReg, $TrashRegister32Bit uxtw #3]

        add         $destReg, $destReg, #0, lsl #0xC
        RELOC       0xA, tls_InlinedThreadStatics                          ;; IMAGE_REL_ARM64_SECREL_HIGH12A
        add         $destReg, $destReg, #0, lsl #0
        RELOC       0x9, tls_InlinedThreadStatics                          ;; IMAGE_REL_ARM64_SECREL_LOW12A

Here $destReg is a native pointer to the location where managed pointer to TLS root is stored.
Nothing needs tracking yet.

        ldr         x19, [$destReg]
           ; gcrRegs +[x19]

Here we have a GC pointer to the managed TLS root

@kunalspathak

Copy link
Copy Markdown
Contributor Author

Fixed to not track the tls_index.

G_M37313_IG03:        ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
            ldr     x0, [xpr, #0x58]
            adrp    x1, [HIGH RELOC #0x4000000000420060]
            add     x1, x1, [LOW RELOC #0x4000000000420060]
            ldr     w1, [x1]
            ldr     x0, [x0, w1, UXTW #3]
            add     x1, x1, [LOW RELOC #0x4000000000420068]
            add     x1, x1, #0
            add     x0, x0, x1
            ldr     x19, [x0]
                             ; gcrRegs +[x19]
            cbz     x19, G_M37313_IG07
@kunalspathak

Copy link
Copy Markdown
Contributor Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).
@VSadov

VSadov commented Jul 17, 2024

Copy link
Copy Markdown
Member

for the

            add     x1, x1, [LOW RELOC #0x4000000000420068]
            add     x1, x1, #0

why the second add has no reloc? Is that just visualization?

@kunalspathak

Copy link
Copy Markdown
Contributor Author

for the

            add     x1, x1, [LOW RELOC #0x4000000000420068]
            add     x1, x1, #0

why the second add has no reloc? Is that just visualization?

yes, fixed it and also fixed the codegen. Now we generate this:

G_M37313_IG03:  ;; offset=0x001C
        F9402E40          ldr     x0, [xpr, #0x58]
        90000001          adrp    x1, [HIGH RELOC #0x4000000000420060]
        91000021          add     x1, x1, [LOW RELOC #0x4000000000420060]
        B9400021          ldr     w1, [x1]
        F8615800          ldr     x0, [x0, w1, UXTW #3]
        91400000          add     x0, x0, [HIGH RELOC #0x4000000000420068]
        91000000          add     x0, x0, [LOW RELOC #0]
        F9400013          ldr     x19, [x0]
        B40000F3          cbz     x19, G_M37313_IG07
@kunalspathak

Copy link
Copy Markdown
Contributor Author

image

@kunalspathak kunalspathak added arm-sve Work related to arm64 SVE/SVE2 support and removed arm-sve Work related to arm64 SVE/SVE2 support labels Jul 18, 2024
@kunalspathak

Copy link
Copy Markdown
Contributor Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).
Comment thread src/coreclr/jit/helperexpansion.cpp Outdated
@kunalspathak
kunalspathak marked this pull request as ready for review July 19, 2024 05:28
@kunalspathak
kunalspathak requested a review from VSadov July 19, 2024 05:28
@kunalspathak

Copy link
Copy Markdown
Contributor Author

@VSadov @dotnet/jit-contrib PTAL

Comment on lines +2761 to +2762
else if (compiler->IsTargetAbi(CORINFO_NATIVEAOT_ABI) && TargetOS::IsWindows &&
op2->IsIconHandle(GTF_ICON_SECREL_OFFSET))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So this constant isn't considered a reloc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is and hence we add EA_CNS_RELOC_FLG | EA_CNS_SEC_RELOC flags below.

@kunalspathak
kunalspathak requested a review from jkotas July 19, 2024 15:10
@kunalspathak

Copy link
Copy Markdown
Contributor Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).
@VSadov

VSadov commented Jul 19, 2024

Copy link
Copy Markdown
Member

failures in System.IO.Packaging are likely related to #105160

@VSadov

VSadov commented Jul 19, 2024

Copy link
Copy Markdown
Member

otherwise windows-arm64 Release does not have any failures

@VSadov

VSadov commented Jul 19, 2024

Copy link
Copy Markdown
Member

I've looked at emitted code in few places and it looks correct.
A typical case:

S_P_CoreLib_System_Threading_Monitor__Enter:
  0000000140494090: A9BD7BFD  stp         fp,lr,[sp,#-0x30]!
  0000000140494094: A901D3F3  stp         x19,x20,[sp,#0x18]
  0000000140494098: F90017F5  str         x21,[sp,#0x28]
  000000014049409C: 910003FD  mov         fp,sp
  00000001404940A0: F9000BBF  str         xzr,[fp,#0x10]
  00000001404940A4: AA0003F3  mov         x19,x0

  // fetching managed TLS root into x20
  00000001404940A8: F9402E40  ldr         x0,[xpr,#0x58]
  00000001404940AC: F000C4C1  adrp        x1,uenum_close_ptr
  00000001404940B0: 91080021  add         x1,x1,#0x200
  00000001404940B4: B9400021  ldr         w1,[x1]
  00000001404940B8: F8615800  ldr         x0,[x0,w1 uxtw #3]
  00000001404940BC: 91400000  add         x0,x0,#0,lsl #0xC
  00000001404940C0: 91004000  add         x0,x0,#0x10
  00000001404940C4: F9400014  ldr         x20,[x0]

  00000001404940C8: B4000BB4  cbz         x20,000000014049423C
. . .
. . .

@VSadov VSadov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!!

@EgorBo

EgorBo commented Jul 19, 2024

Copy link
Copy Markdown
Member

Sorry, I didn't realize your PR bumped the JIT-EE guid and merged a community PR with jit-ee guid bump. I think you can probably resolve the conflict and merge without waiting for CI..

@kunalspathak

Copy link
Copy Markdown
Contributor Author

/ba-g failures are unrelated

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

4 participants