t.p.northover marked 6 inline comments as done.
t.p.northover added a comment.
Thanks, I've updated for most of the suggestions and committed it. I'll make
the AArch64 naming changes separately if we decide to.
================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:167
// Target properties.
- if (!getTriple().isOSWindows()) {
+ if (!getTriple().isOSWindows() && getTriple().isArch64Bit()) {
Builder.defineMacro("_LP64");
----------------
jfb wrote:
> 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.
isArch64Bit is taken directly from Triple::aarch64/Triple::aarch64_32, so I
think someone would have to be intentionally bringing up an aarch64_32 platform
to be affected, in which case they probably want to be.
================
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 &&
----------------
jfb wrote:
> The comment isn't quite right anymore. Maybe don't say `AArch64` since the
> code is obvious about what it checks?
AArch64 is the official name of the 64-bit execution mode of ARM processors so
I think it's still correct to say aarch64_32 is AArch64. Or were you referring
to some other aspect of the comment?
================
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();
----------------
jfb wrote:
> This is now a weird variable name, since it's aarch64 maybe 32 but not be.
> Could you rename `IsAArch64`?
I think the same point about the comment above applies here. The Triple is the
odd thing out here in distinguishing the two.
================
Comment at: clang/test/CodeGen/builtins-arm64.c:11
+#if __LP64__
void *tp (void) {
----------------
jfb wrote:
> Why isn't this one supported?
It was an iOS version check, I've updated the triple instead.
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
