Page MenuHomePhabricator

TomTan (Tom Tan)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 19 2018, 12:39 PM (60 w, 3 d)

Recent Activity

Jun 21 2019

TomTan abandoned D63617: [COFF, ARM64] Fix encoding of __debugbreak.

The fix in LLVM was merged as https://reviews.llvm.org/rL364115.

Jun 21 2019, 4:46 PM · Restricted Project
TomTan committed rG7ecb5145bae5: [COFF, ARM64] Fix encoding of debugtrap for Windows (authored by TomTan).
[COFF, ARM64] Fix encoding of debugtrap for Windows
Jun 21 2019, 4:42 PM
TomTan committed rL364115: [COFF, ARM64] Fix encoding of debugtrap for Windows.
[COFF, ARM64] Fix encoding of debugtrap for Windows
Jun 21 2019, 4:42 PM
TomTan closed D63635: [COFF, ARM64] Fix encoding of debugtrap for Windows.
Jun 21 2019, 4:42 PM · Restricted Project
TomTan updated the diff for D63635: [COFF, ARM64] Fix encoding of debugtrap for Windows.

Thanks Reid and Eli for the catch. Added -fast-isel and -global-isel to test configurations, and also added the implementation for both fast isel and global isel.

Jun 21 2019, 2:38 PM · Restricted Project

Jun 20 2019

TomTan added a comment to D63617: [COFF, ARM64] Fix encoding of __debugbreak.
In D63617#1552615, @rnk wrote:

Even if BRK #0xF000 is a Windows convention, it's still possible for ISel to select different instructions for different OSs, and I'd prefer to implement it that way.

Jun 20 2019, 5:56 PM · Restricted Project
TomTan created D63635: [COFF, ARM64] Fix encoding of debugtrap for Windows.
Jun 20 2019, 5:55 PM · Restricted Project
TomTan added a comment to D63617: [COFF, ARM64] Fix encoding of __debugbreak.
In D63617#1552561, @rnk wrote:

I think it would be preferable to make llvm.debugtrap emit brk #0xF000 on aarch64-windows-*, so other frontends (Rust etc) get the right behavior by default. Right now, AArch64 doesn't do anything special for DEBUGTRAP, so we get the default behavior of llvm.trap.

Is this BRK #0xF000 convention universal to all OSs, or is it a specific convention for Windows debuggers?

Jun 20 2019, 1:38 PM · Restricted Project
TomTan created D63617: [COFF, ARM64] Fix encoding of __debugbreak.
Jun 20 2019, 12:25 PM · Restricted Project

Jun 3 2019

TomTan added a comment to D62608: [ARM64, COFF] Add CodeView register mapping.

Could we, please, revert this until the fix for LLDB is ready and approved?

The fix for LLDB was merged yesterday.

Jun 3 2019, 10:23 AM · Restricted Project

Jun 2 2019

TomTan added a comment to D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames.

This should get the build working again, so lets get this fixed, we can improve it later

Jun 2 2019, 5:53 PM · Restricted Project, Restricted Project
TomTan committed rG382320ea025f: [COFF, ARM64] Fix CodeView API change for getRegisterNames (authored by TomTan).
[COFF, ARM64] Fix CodeView API change for getRegisterNames
Jun 2 2019, 5:47 PM
TomTan committed rL362349: [COFF, ARM64] Fix CodeView API change for getRegisterNames.
[COFF, ARM64] Fix CodeView API change for getRegisterNames
Jun 2 2019, 5:47 PM
TomTan closed D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames.
Jun 2 2019, 5:47 PM · Restricted Project, Restricted Project
TomTan added inline comments to D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames.
Jun 2 2019, 10:57 AM · Restricted Project, Restricted Project
TomTan updated the diff for D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames.

Change to switch/case based on comment.

Jun 2 2019, 10:56 AM · Restricted Project, Restricted Project
TomTan added inline comments to D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames.
Jun 2 2019, 10:50 AM · Restricted Project, Restricted Project

Jun 1 2019

TomTan added a comment to D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames.

Are you sure this triple → CPUType mapping belongs in each consumer? Maybe it'd be better to have something inside LLVM, so that we wouldn't have to keep this up-to-date in all the places. Maybe getRegisterNames() overload that takes a triple?

Jun 1 2019, 1:17 AM · Restricted Project, Restricted Project
TomTan updated the diff for D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames.

Update variable naming to be consistent with current file.

Jun 1 2019, 1:06 AM · Restricted Project, Restricted Project
TomTan added a comment to D62608: [ARM64, COFF] Add CodeView register mapping.
Jun 1 2019, 12:52 AM · Restricted Project
TomTan created D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames.
Jun 1 2019, 12:46 AM · Restricted Project, Restricted Project

May 31 2019

TomTan committed rG2258ecc2aaef: [COFF, ARM64] Fix location of ARM64 CodeView test (authored by TomTan).
[COFF, ARM64] Fix location of ARM64 CodeView test
May 31 2019, 7:36 PM
TomTan committed rL362283: [COFF, ARM64] Fix location of ARM64 CodeView test.
[COFF, ARM64] Fix location of ARM64 CodeView test
May 31 2019, 7:35 PM
TomTan committed rGeb4d6142dcd5: [COFF, ARM64] Add CodeView register mapping (authored by TomTan).
[COFF, ARM64] Add CodeView register mapping
May 31 2019, 4:41 PM
TomTan committed rL362280: [COFF, ARM64] Add CodeView register mapping.
[COFF, ARM64] Add CodeView register mapping
May 31 2019, 4:41 PM
TomTan closed D62608: [ARM64, COFF] Add CodeView register mapping.
May 31 2019, 4:41 PM · Restricted Project
TomTan updated the diff for D62608: [ARM64, COFF] Add CodeView register mapping.

Remove unnecessary const and clean up.

May 31 2019, 2:49 PM · Restricted Project
TomTan added inline comments to D62608: [ARM64, COFF] Add CodeView register mapping.
May 31 2019, 2:17 PM · Restricted Project
TomTan updated the diff for D62608: [ARM64, COFF] Add CodeView register mapping.

It makes sense to fix the dumper for test infrastructure. This update fixes register names in the dumper. I also changed the wildcard for register name to ARM64_SP in the new test, because register name is shown correct by the dumper based on CPUType in the PDB.

May 31 2019, 12:12 PM · Restricted Project

May 29 2019

TomTan added a comment to D62608: [ARM64, COFF] Add CodeView register mapping.
In D62608#1522078, @rnk wrote:

The code looks fine now. I'm okay with putting off fixes for dumping code that incorrectly assumes x86.

The dumping tools are the foundation of our testing strategy, so I think we should take the time to fix them early on. It should be a matter of changing getRegisterNames to accept a CPU type, switch, and return the appropriate (x86 or arm) enum table, maybe just returning x86 by default to preserve existing behavior. Both symbol dumpers already store the compilation CPU type from the S_COMPILE3 record, so this shouldn't be too complicated.

May 29 2019, 2:49 PM · Restricted Project
TomTan updated the diff for D62608: [ARM64, COFF] Add CodeView register mapping.

Did a little cleanup as per Eli's comment (removed attributes and llvm.ident. Tried some others but got llc failure, so I'd like to be conservation on this. Also the test follows register-variables.ll in the same folder which keeps all the metadata.

May 29 2019, 2:22 PM · Restricted Project
TomTan updated the diff for D62608: [ARM64, COFF] Add CodeView register mapping.

Added CV_REGISTERS_{ARCH} macro for each architecture, and fixed typo.

May 29 2019, 1:09 PM · Restricted Project
TomTan created D62608: [ARM64, COFF] Add CodeView register mapping.
May 29 2019, 9:10 AM · Restricted Project

May 1 2019

TomTan committed rGb7c6d95af5e2: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI (authored by TomTan).
[COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI
May 1 2019, 5:38 PM
TomTan committed rL359744: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI.
[COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI
May 1 2019, 5:38 PM
TomTan committed rC359744: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI.
[COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI
May 1 2019, 5:37 PM
TomTan closed D61225: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI.
May 1 2019, 5:37 PM · Restricted Project, Restricted Project

Apr 29 2019

TomTan updated the diff for D61225: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI.

Added test cases and also merged this alignment adjustment to getMinGlobalAlign in MicrosoftARM64TargetInfo.

Apr 29 2019, 3:55 PM · Restricted Project, Restricted Project

Apr 27 2019

TomTan created D61225: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI.
Apr 27 2019, 8:36 AM · Restricted Project, Restricted Project

Apr 25 2019

TomTan added a comment to D60349: [COFF, ARM64] Fix ABI implementation of struct returns.

Return info for HFA and HVA is updated

That's helpful, but not really sufficient; according to the AAPCS rules, both "Pod" and "NotPod" from my testcase are HFAs.

Apr 25 2019, 4:25 PM · Restricted Project
TomTan added a comment to D60349: [COFF, ARM64] Fix ABI implementation of struct returns.

For NotPod, it is aggregate which is specific in the document

Yes, it's an aggregate which is returned in registers... but it's returned in integer registers, unlike Pod which is returned in floating-point registers.

struct Pod is HFA (Homogenous Floating-Point Aggregates) which is returned in floating-point register, struct NotPod is not HFA so still returned in integer registers. The ARM64 ABI document will be updated to reflect this, thanks.

Apr 25 2019, 3:20 PM · Restricted Project

Apr 24 2019

TomTan added a comment to D60349: [COFF, ARM64] Fix ABI implementation of struct returns.

For NotPod, it is aggregate which is specific in the document

Yes, it's an aggregate which is returned in registers... but it's returned in integer registers, unlike Pod which is returned in floating-point registers.

Apr 24 2019, 3:10 PM · Restricted Project

Apr 23 2019

TomTan added a comment to D60349: [COFF, ARM64] Fix ABI implementation of struct returns.

It looks like there's some missing documentation in the ARM64 ABI document involving homogeneous aggregates... in particular, it looks like non-POD types are never homogeneous, or something along those lines. I guess we can address that in a followup, though.

@TomTan could you look into updating the ARM64 ABI documentation?

Testcase:

struct Pod {
  double b[2];
};
struct NotAggregate {
  NotAggregate();
  double b[2];
};
struct NotPod {
  NotAggregate x;
};
Pod copy(Pod *x) { return *x; }  // ldp d0,d1,[x0]
NotAggregate copy(NotAggregate *x) { return *x; } // stp x8,x9,[x0]
NotPod copy(NotPod *x) { return *x; } // ldp x0,x1,[x8]
Apr 23 2019, 9:31 PM · Restricted Project

Feb 11 2019

TomTan added a comment to D57915: [COFF, ARM64] Remove definitions for _byteswap library functions.

Added the tests back. Clang IR should not lower these to bswap calls because they are global library functions. It might be slower to make the call to library function than bswap, but this is the same for other architectures supported by Windows. And just redefine global library function triggers link error in some scenarios.

I think there is some deeper reasoning is being omitted here.
Why does the fact that those are "global library functions" bans clang from lowering them into IR?
(and thus, "prevents" LLVM form doing optimizations)

The current issue could be caused by the implementation of comdat selection in LLD as below which provides more strict conflict check than MSVC link does.

lld isn't supposed to be more strict than link.exe. lld used to not implement the comdat selection field until recently, so lld got more strict – but it should've gotten only as strict as link.exe, not moreso. Do you have an example where lld is more strict than link.exe?

Feb 11 2019, 12:35 PM · Restricted Project, Restricted Project
TomTan committed rG42b2424e4fa2: [COFF, ARM64] Remove definitions for _byteswap library functions (authored by TomTan).
[COFF, ARM64] Remove definitions for _byteswap library functions
Feb 11 2019, 12:04 PM
TomTan committed rC353740: [COFF, ARM64] Remove definitions for _byteswap library functions.
[COFF, ARM64] Remove definitions for _byteswap library functions
Feb 11 2019, 12:03 PM
TomTan committed rL353740: [COFF, ARM64] Remove definitions for _byteswap library functions.
[COFF, ARM64] Remove definitions for _byteswap library functions
Feb 11 2019, 12:03 PM
TomTan closed D57915: [COFF, ARM64] Remove definitions for _byteswap library functions.
Feb 11 2019, 12:03 PM · Restricted Project, Restricted Project

Feb 7 2019

TomTan added a comment to D57915: [COFF, ARM64] Remove definitions for _byteswap library functions.

I did some quick testing with MSVC; apparently it inlines the implementations of these functions when optimizations are on. We definitely want to support inlining these. Since these are commonly used in performance-sensitive code, I'd prefer to implement the required changes to CodeGenFunction::EmitMSVCBuiltinExpr now, rather than chase after weird performance regressions in the future.


I'm not sure how you could end up with a "duplicate symbols" error from the current implementation, though; these functions are marked "static", so they shouldn't conflict with the real _byteswap_* functions.

Feb 7 2019, 3:31 PM · Restricted Project, Restricted Project
TomTan added a comment to D57915: [COFF, ARM64] Remove definitions for _byteswap library functions.

Added the tests back. Clang IR should not lower these to bswap calls because they are global library functions. It might be slower to make the call to library function than bswap, but this is the same for other architectures supported by Windows. And just redefine global library function triggers link error in some scenarios.

I think there is some deeper reasoning is being omitted here.
Why does the fact that those are "global library functions" bans clang from lowering them into IR?
(and thus, "prevents" LLVM form doing optimizations)

The current issue could be caused by the implementation of comdat selection in LLD as below which provides more strict conflict check than MSVC link does.

These _bytewap_* in intrin.h were not intended as optimization, instead, it is expected to be consistent with declarations in stdlib.h. For optimization, it makes sense to enable it for all architectures supported by Windows, and make sure it works with LLD.

https://github.com/llvm-mirror/lld/blob/b6584c3fab115aa46dad27681af7eb3d4e5d2b35/COFF/InputFiles.cpp#L494

These functions are also listed in https://github.com/llvm-mirror/clang/blob/c18e7e9007970a3105617f03bc9d1de89fa1a3e1/include/clang/Basic/Builtins.def#L773-L775
Are you sure they shouldn't be simply dropped from lib/Headers/intrin.h?

I notice they were just added in D56685, but that review has no commit message, so the problem it addressed is not documented..
So as-is this all looks a bit like hand-waving to me.

Feb 7 2019, 3:22 PM · Restricted Project, Restricted Project
TomTan added a comment to D57915: [COFF, ARM64] Remove definitions for _byteswap library functions.

Added the tests back. Clang IR should not lower these to bswap calls because they are global library functions. It might be slower to make the call to library function than bswap, but this is the same for other architectures supported by Windows. And just redefine global library function triggers link error in some scenarios.

I think there is some deeper reasoning is being omitted here.
Why does the fact that those are "global library functions" bans clang from lowering them into IR?
(and thus, "prevents" LLVM form doing optimizations)

Feb 7 2019, 2:36 PM · Restricted Project, Restricted Project
TomTan updated the diff for D57915: [COFF, ARM64] Remove definitions for _byteswap library functions.

Added the tests back. Clang IR should not lower these to bswap calls because they are global library functions. It might be slower to make the call to library function than bswap, but this is the same for other architectures supported by Windows. And just redefine global library function triggers link error in some scenarios.

Feb 7 2019, 1:04 PM · Restricted Project, Restricted Project
TomTan created D57915: [COFF, ARM64] Remove definitions for _byteswap library functions.
Feb 7 2019, 11:01 AM · Restricted Project, Restricted Project

Feb 6 2019

TomTan committed rGdcb9e08fae43: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail (authored by TomTan).
[COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail
Feb 6 2019, 12:09 PM
TomTan committed rC353337: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail.
[COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail
Feb 6 2019, 12:08 PM
TomTan committed rL353337: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail.
[COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail
Feb 6 2019, 12:08 PM
TomTan closed D57631: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail.
Feb 6 2019, 12:08 PM · Restricted Project, Restricted Project

Feb 4 2019

TomTan added a comment to D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg.

Thanks for finding out and fixing this. Seems there is also issue in expanding _WriteStatusReg in CodeGenFunction::EmitAArch64BuiltinExpr. The last argument for _WriteStatusReg is zero extended to __in64, which is not expected (see below link).

Feb 4 2019, 12:15 PM · Restricted Project, Restricted Project
TomTan added a comment to D57631: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail.

LGTM

Feb 4 2019, 12:06 PM · Restricted Project, Restricted Project

Feb 1 2019

TomTan created D57631: [COFF, ARM64] Add ARM64 support for MS intrinsic _fastfail.
Feb 1 2019, 4:43 PM · Restricted Project, Restricted Project
TomTan added inline comments to D57575: [LLD] [COFF] Create range extension thunks for ARM64.
Feb 1 2019, 10:52 AM · Restricted Project

Jan 30 2019

TomTan added a comment to D57183: [COFF, ARM64] Fix localaddress to handle stack realignment and variable size objects.
Jan 30 2019, 4:56 PM · Restricted Project

Jan 28 2019

TomTan added inline comments to D57277: [COFF, ARM64] Don't put jump table into a separate COFF section for EK_LabelDifference32.
Jan 28 2019, 4:46 PM
TomTan updated the diff for D57277: [COFF, ARM64] Don't put jump table into a separate COFF section for EK_LabelDifference32.

Added COFF target check.

Jan 28 2019, 4:43 PM
TomTan added inline comments to D57277: [COFF, ARM64] Don't put jump table into a separate COFF section for EK_LabelDifference32.
Jan 28 2019, 3:03 PM

Jan 27 2019

TomTan added a comment to D57277: [COFF, ARM64] Don't put jump table into a separate COFF section for EK_LabelDifference32.

Thanks for the review, Martin. Could you please help commit this? I don't have commit access.

Jan 27 2019, 1:09 PM

Jan 26 2019

TomTan updated the diff for D57277: [COFF, ARM64] Don't put jump table into a separate COFF section for EK_LabelDifference32.

Added CHECK-NEXT on the jump table label based on Martin's feedback.

Jan 26 2019, 7:45 PM
TomTan added a comment to D57277: [COFF, ARM64] Don't put jump table into a separate COFF section for EK_LabelDifference32.

Thanks Peter and Martin. Yes, IMAGE_REL_ARM64_REL32 can do the relocation work here. I saw Martin added this relocation type to both LLVM and LLD. My change in AArch64AsmPrinter::EmitJumpTableInfo could be reverted when IMAGE_REL_ARM64_REL32 is attached to the jump table entries, but the relocation model is PIC for Windows ARM64.

Jan 26 2019, 7:33 PM

Jan 25 2019

TomTan created D57277: [COFF, ARM64] Don't put jump table into a separate COFF section for EK_LabelDifference32.
Jan 25 2019, 6:06 PM

Jan 16 2019

TomTan added a comment to D56813: [AArch64] [Windows] Share unwind codes between epilogues with identical unwind codes.

I've confirmed that I can build Firefox successfully with this patch on top of latest trunk. Thank you @ssijaric!

Hi David,

Can you please try with the latest version of the patch? I am going to commit it shortly.

Thanks.

Jan 16 2019, 5:22 PM

Jan 11 2019

TomTan added a comment to D56620: [COFF, ARM64] Declare intrinsics: __nop, _byteswap_[ushort/ulong/uint64].

We need full definition for __nop in intrin.h.

Jan 11 2019, 5:15 PM

Jan 2 2019

TomTan added a comment to D30526: [Support] Add functions to get and set thread name..

Hi Zachary, does this line "#include "Windows/WindowsSupport.h" " need to be changed to "#include "WindowsSupport.h" " after it is moved from Threading.cpp to Threading.inc, because Threading.inc which includes WindowsSupport.h is under Windows folder, so no need to have it in the include path. This current include path could trigger warning "#include resolved using non-portable Microsoft search rules as: ..."

Jan 2 2019, 4:17 PM

Dec 10 2018

TomTan requested changes to D53540: [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally.

@dmajor My updated change fixes the crash in your testcase.

Now, I am trying to build the SEH tests released by MS: https://github.com/Microsoft/windows_seh_tests. I started with XCPT4 tests. test5 is currently failing which I am triaging now.

Dec 10 2018, 4:28 PM

Nov 8 2018

TomTan added inline comments to D53540: [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally.
Nov 8 2018, 5:46 PM

Jul 19 2018

TomTan added inline comments to D49464: [COFF, ARM64] Decide when to mark struct returns as SRet.
Jul 19 2018, 2:50 PM