peter.smith marked 7 inline comments as done. peter.smith added a comment. Thanks very much for the comments. I'll post an update shortly.
================ Comment at: lib/Driver/ToolChains/Gnu.cpp:357-364 + const char* EndianFlag = "-EL"; + if (isArmBigEndian(Triple, Args)) { + EndianFlag = "-EB"; + arm::appendBE8LinkFlag(Args, CmdArgs, Triple); + } + else if (Arch == llvm::Triple::aarch64_be) + EndianFlag = "-EB"; ---------------- nickdesaulniers wrote: > nickdesaulniers wrote: > > ``` > > bool IsBigEndian = isArmBigEndian(Triple, Args); > > if (IsBigEndian) > > arm::appendBE8LinkFlag(Args, CmdArgs, Triple); > > IsBigEndian |= Arch == llvm::Triple::aarch64_be; > > CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL"); > > ``` > `IsBigEndian |= Arch == llvm::Triple::aarch64_be;` > > should be: > > `IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;` > > in order to not evaluate `Arch == llvm::Triple::aarch64_b` if `IsBigEndian` > is already true. Thanks for the suggestion. One thing it highlighted was that isArmBigEndian could return true for an aarch64_be arch with -mbig-endian so I've rewritten isArmBigEndian to always return false if the architecture isn't Arm and have added some test cases to check that "--be8" doesn't sneak in. ================ Comment at: lib/Driver/ToolChains/Gnu.cpp:362 + } + else if (Arch == llvm::Triple::aarch64_be) + EndianFlag = "-EB"; ---------------- nickdesaulniers wrote: > is having the `else if` on its own line what the formatter chose? I'd forgot to run clang-format over that part of the code. I've adopted the snippet below which replaces it. ================ Comment at: lib/Driver/ToolChains/Gnu.cpp:703 case llvm::Triple::aarch64_be: { + if (getToolChain().getTriple().isLittleEndian()) + CmdArgs.push_back("-EL"); ---------------- nickdesaulniers wrote: > earlier (L362), you check the endianess of the triple with: > > ``` > Arch == llvm::Triple::aarch64_be > ``` > where `Arch` is `ToolChain.getArch()`. > > I don't have a preference, but these two seem inconsistent. Can we either > check the explicit `llvm::Triple::` or call > `getToolChain().getTriple().isLittleEndian()` in both, rather than mix? I originally took that part from the Mips code, I've replaced it with a check against aarch64_be which is more consistent with the other Arm and AArch64 code. https://reviews.llvm.org/D52784 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits