jfb accepted this revision.
jfb added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: dexonsmith.
Minor comments, but otherwise LGTM.
================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:167
// Target properties.
- if (!getTriple().isOSWindows()) {
+ if (!getTriple().isOSWindows() && getTriple().isArch64Bit()) {
Builder.defineMacro("_LP64");
----------------
This might affect odd non-Darwin targets? Seems unlikely, but just asking since
we have existence proof with Windows that stuff is weird. Admittedly they're
untested if it affects them, so I think this is fine.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5457
// for AArch64, emit a warning and ignore the flag. Otherwise, add the
// proper mllvm flags.
+ if (Triple.getArch() != llvm::Triple::aarch64 &&
----------------
The comment isn't quite right anymore. Maybe don't say `AArch64` since the code
is obvious about what it checks?
================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:59
.Cases("armv7s", "xscale", llvm::Triple::arm)
- .Case("arm64", llvm::Triple::aarch64)
+ .Case("arm64", llvm::Triple::aarch64)
+ .Case("arm64_32", llvm::Triple::aarch64_32)
----------------
Extra space.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:5512
+ bool IsAArch64 = (TT.getArch() == llvm::Triple::aarch64 ||
+ TT.getArch() == llvm::Triple::aarch64_32);
bool IsWindows = TT.isOSWindows();
----------------
This is now a weird variable name, since it's aarch64 maybe 32 but not be.
Could you rename `IsAArch64`?
================
Comment at: clang/test/CodeGen/builtins-arm64.c:11
+#if __LP64__
void *tp (void) {
----------------
Why isn't this one supported?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63131/new/
https://reviews.llvm.org/D63131
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
