Page MenuHomePhabricator

[MSVC, ARM64] Add __writex18 intrinsics
ClosedPublic

Authored by steplong on May 19 2022, 3:09 PM.

Details

Summary

https://docs.microsoft.com/en-us/cpp/intrinsics/arm64-intrinsics?view=msvc-170

void __writex18byte(unsigned long, unsigned char)
void __writex18word(unsigned long, unsigned short)
void __writex18dword(unsigned long, unsigned long)
void __writex18qword(unsigned long, unsigned __int64)

Given the lack of documentation of the intrinsics, we chose to align the offset with just
CharUnits::One() when calling IRBuilderBase::CreateAlignedStore().

Diff Detail

Event Timeline

steplong created this revision.May 19 2022, 3:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 3:09 PM
steplong requested review of this revision.May 19 2022, 3:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 3:09 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

The generated assembly is a little different from MSVC's:

MSVC                                                                          LLVM
__writex18byte:
                          strb    w1, [x18, x0]                             strb    w1, [x18, w0, uxtw]
__writex18word:
                          strh    w1, [x18, x0]                             strh    w1, [x18, w0, uxtw #1]
__writex18dword:
                          str     w1, [x18, x0]                              str     w1, [x18, w0, uxtw #2]
__writex18qword:
                          str     x1, [x18, x0]                              str     x1, [x18, w0, uxtw #3]
steplong updated this revision to Diff 430831.May 19 2022, 3:30 PM
  • Rebased it on top of main instead of D126024

uxtw #3 makes me think you're not generating the right code... the zero-extension hopefully doesn't matter, but the shift is significant. Maybe should be generating getelementptr i8?

steplong updated this revision to Diff 430979.May 20 2022, 8:12 AM

The assembly for LLVM now looks like:

__writex18byte:
                        strb    w1, [x18, w0, uxtw]
                        ret

__writex18word:
                        strh    w1, [x18, w0, uxtw]
                        ret

__writex18dword:
                        str     w1, [x18, w0, uxtw]
                        ret

__writex18qword:
                        str     x1, [x18, w0, uxtw]
                        ret
rnk added a comment.May 20 2022, 10:29 AM

I was unable to find any documentation for the meaning of AArch64 addrspace(256), and I wasn't able to figure it out after studying the code in llvm/lib/Target/AArch64 for ten minutes or so. The change seems fine, but please add some documentation as a follow-up. X86 has some of its address spaces documented here, not that this is the best place:
https://llvm.org/docs/CodeGenerator.html#x86-address-spaces-supported

I was unable to find any documentation for the meaning of AArch64 addrspace(256), and I wasn't able to figure it out after studying the code in llvm/lib/Target/AArch64 for ten minutes or so. The change seems fine, but please add some documentation as a follow-up. X86 has some of its address spaces documented here, not that this is the best place:
https://llvm.org/docs/CodeGenerator.html#x86-address-spaces-supported

Hmm yea I don't think addrspace(256) is correct. I was using D31248 as a reference and addrspace(256) makes sense for that patch.

steplong updated this revision to Diff 431013.May 20 2022, 11:22 AM
  • Changed addrspace(256) to the default addrspace 0
efriedma added inline comments.May 20 2022, 11:51 AM
clang/lib/CodeGen/CGBuiltin.cpp
9977

I think I'd prefer just "CharUnits::One()", rather than getTypeAlignInChars(); given the lack of documentation, it's not clear if the offset is required to be properly aligned.

steplong updated this revision to Diff 431019.May 20 2022, 11:58 AM
  • Switch to CharUnits::One() instead of getContext().getTypeInChars()
This revision is now accepted and ready to land.May 20 2022, 2:48 PM
steplong edited the summary of this revision. (Show Details)May 23 2022, 6:59 AM
steplong edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.