[PATCH] D49674: [AArch64] Add Tiny Code Model for AArch64

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. Looks good to me now that LLVM support has been approved in https://reviews.llvm.org/D49673 https://reviews.llvm.org/D49674 ___ cfe-co

[PATCH] D46932: [AArch64] Correct inline assembly test case for S modifier [NFC]

peter.smith created this revision. peter.smith added reviewers: t.p.northover, manojgupta. Herald added subscribers: kristof.beyls, eraman, rengolin. Herald added a reviewer: javed.absar. The existing test for the AArch64 constraint S uses the A and L modifiers. These modifiers were implemented i

[PATCH] D46932: [AArch64] Correct inline assembly test case for S modifier [NFC]

This revision was automatically updated to reflect the committed changes. Closed by commit rC332606: [AArch64] Correct inline assembly test case for S modifier [NFC] (authored by psmith, committed by ). Repository: rC Clang https://reviews.llvm.org/D46932 Files: test/CodeGen/aarch64-inline-

[PATCH] D39179: [AArch64] Fix PR34625 -mtune without -mcpu should not set -target-cpu

peter.smith created this revision. Herald added subscribers: javed.absar, rengolin, aemerson. When -mtune is used on AArch64 the -target-cpu is passed the value of the cpu given to -mtune. As well as setting micro-architectural features of the -mtune cpu, this will also add the architectural fea

[PATCH] D39179: [AArch64] Fix PR34625 -mtune without -mcpu should not set -target-cpu

This revision was automatically updated to reflect the committed changes. Closed by commit rL316424: [AArch64] Fix PR34625 -mtune without -mcpu should not set -target-cpu (authored by psmith). Changed prior to commit: https://reviews.llvm.org/D39179?vs=119837&id=120025#toc Repository: rL LLV

[PATCH] D34513: [NFC] Update to account for DiagnosticRenderer use of FullSourceLoc

peter.smith created this revision. https://reviews.llvm.org/D31709 [NFC] Refactor DiagnosticRenderer to use FullSourceLoc was committed in r305684 and reverted in 305688 as clang-tidy and clang-query failed to build. This change updates the extra tools to use the new interface. With this chang

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

peter.smith added a comment. Ping. Does anyone have any changes they'd like me to make? https://reviews.llvm.org/D52784 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53121: [Driver] Add defaults for Android ARM FPUs.

peter.smith added a comment. It looks like we might be able to simplify a bit, and I think there probably could be some more test cases but no objections from me. Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:360 + unsigned ArchVersion; + if (ArchName.empty()) ---

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

peter.smith added a comment. Thanks for the comments. I agree with you that the -EB and -EL flags need to be passed to the linker as well, I kind of expected that ld.bfd would infer it from the endianness of the first object but it doesn't seem to do that. If it's ok I'll do that in a separate

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

peter.smith added a comment. On reflection after looking at what would be needed for the linker tools::gnutools::Linker::ConstructJob() I think it may be better to move the triple detection into a separate function. I'll work on that and will hopefully post an update soon. https://reviews.llv

[PATCH] D53121: [Driver] Add defaults for Android ARM FPUs.

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. Looks good to me. By generic target I just meant --target=arm-linux-androideabi21 with a -march to specify the revision, which you've got in the test. Repository: rC Clang https

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

peter.smith updated this revision to Diff 169386. peter.smith added a comment. I've decided to roll the linker changes in with the assembler ones as the linker use case affects the design. It turns out that only Arm needs to check to see if the -mbig-endian and -mlittle-endian flags override the

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

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)) { + Endi

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

peter.smith updated this revision to Diff 169689. peter.smith marked 3 inline comments as done. peter.smith added a comment. Updated diff to reflect review comments. Main changes are: - isArmBigEndian always returns false if the target architecture isn't Arm. - Added tests to make sure "--be8" do

[PATCH] D53272: Add target requirement to profile remap test.

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. Looks good to me. REQUIRES: x86-registered-target is used in the same directory for tests that depend on specific targets. Repository: rC Clang https://reviews.llvm.org/D53272

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

This revision was automatically updated to reflect the committed changes. Closed by commit rC344597: [ARM][AArch64] Pass through endian flags to assembler and linker. (authored by psmith, committed by ). Changed prior to commit: https://reviews.llvm.org/D52784?vs=169689&id=169799#toc Repositor

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

peter.smith added a comment. In https://reviews.llvm.org/D45240#1267846, @stefson wrote: > hey there, I've run into problems with stripping static-libs on arm when > using llvm/clang-7. Could you imagine this patch being at fault? > > strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneede

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

peter.smith added a comment. Thanks for pointing that out, I'm out of office today, will look at describing the intention to fall through when I get back in on Monday. Comment at: lib/Driver/ToolChains/Gnu.cpp:241 + case llvm::Triple::thumbeb: +IsBigEndian = true; + case

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

peter.smith added a comment. Added LLVM_FALLTHROUGH; in r344890. Repository: rC Clang https://reviews.llvm.org/D52784 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

peter.smith added a comment. I think we need to be very careful here. If I've understood this correctly then if this functionality is used for anything critical then a manually supplied target will be needed when doing cross-linking. For example my host LLD is x86_64, is just called ld.lld and

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

peter.smith added a comment. Yes this is fine. The effects are entirely within the Android target. The change in LLD to a 64k page size was made in D25079 . The main reason given was that a sufficient number of linux distributions including Redhat had chosen a 6

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

peter.smith added a comment. In D55029#1313120 , @ruiu wrote: > LGTM. Please commit. > > Peter, I wonder if you are fine with the default 64KiB page size with lld, > especially given that lld always round up the text segment size to the > maximum page si

[PATCH] D58320: [Darwin] Introduce a new flag, -fapple-link-rtlib that forces linking of the builtins library.

peter.smith added a comment. I've no objections to adding the command line as a Darwin only option. Implementation looks fine to me although I've not got any prior experience with Darwin. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58320/new/ h

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

peter.smith added a comment. I'm not in favour of adding AArch64 support to -mcrypto -mnocrypto for a few reasons: - Arm are trying to keep the options for controlling target features as consistent as possible with GCC and this option isn't supported in GCC (https://gcc.gnu.org/onlinedocs/gcc/

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

peter.smith added a comment. In D60472#1461336 , @manojgupta wrote: > The motivation for this change is to make "crypto" setting an additive option > e.g. like "-mavx" used in many media packages. Some packages in Chrome want > to enable crypto conditio

[PATCH] D52595: [ARM] Alter test to account for change to armv6k default CPU

peter.smith created this revision. peter.smith added reviewers: falstaff84, nickdesaulniers, rengolin, labrinea. Herald added a reviewer: javed.absar. Herald added subscribers: chrib, kristof.beyls. Review https://reviews.llvm.org/D52594 will change the default in llvm for armv6k from the non-exi

[PATCH] D52595: [ARM] Alter test to account for change to armv6k default CPU

This revision was automatically updated to reflect the committed changes. Closed by commit rL343304: [ARM] Alter test to account for change to armv6k default CPU (authored by psmith, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D5259

[PATCH] D52595: [ARM] Alter test to account for change to armv6k default CPU

This revision was automatically updated to reflect the committed changes. Closed by commit rC343304: [ARM] Alter test to account for change to armv6k default CPU (authored by psmith, committed by ). Repository: rC Clang https://reviews.llvm.org/D52595 Files: test/Driver/arm-cortex-cpus.c

[PATCH] D52705: First-pass at ARM hard/soft-float multilib driver (NOT WORKING YET)

peter.smith added a comment. I think that you may be better off posting a RFC to llvm-dev to get discussion on the best approach here. It would also be good to step back a bit and consider the requirements, as it stands it looks like this might be a solution for just one particular multilib lay

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

peter.smith created this revision. peter.smith added reviewers: rengolin, compnerd, olista01. Herald added subscribers: chrib, kristof.beyls, srhines. Herald added a reviewer: javed.absar. The big-endian arm32 Linux builds are currently failing when the -mbig-endian and -fno-use-integrated-as fla

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

peter.smith added a comment. In https://reviews.llvm.org/D52784#1252569, @rengolin wrote: > Hi Peter, > > Looks ok. Why did you need to change all cmdlines to have -EL? I imagine you > just need one for each case, everything else remains the default (which > should still work). I thought that

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

peter.smith updated this revision to Diff 167955. peter.smith added a comment. Updated diff to add more tests, including some that use -mlittle-endian when the target is big-endian. https://reviews.llvm.org/D52784 Files: lib/Driver/ToolChains/Gnu.cpp test/Driver/linux-as.c Index: test/Dri

[PATCH] D58320: [Darwin] Introduce a new flag, -flink-builtins-rt that forces linking of the builtins library.

peter.smith added a comment. The implementation changes in the Darwin toolchain look fine to me, although with respect to the command line option I think Petr Hosek's message on cfe-dev is interesting: > GCC implements -nolibc which could be used to achieve the same effect when > combined with

[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

peter.smith added a comment. My main concern is that this changes the default behaviour for arm-linux-gnueabi and arm-linux-gnueabihf targets. I've put some suggestions on what I think the behaviour should be. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:277 + } + r

[PATCH] D58429: [CodeGen] Enable the complex-math test for arm

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM. Thanks for the fix. My apologies for missing that at the time, it looks like a cut and paste error on my part. Repository: rC Clang CHANGES SINCE LAST ACTION https://revi

[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

peter.smith added a comment. Thanks for the update. To summarise I'd like to keep the integrated and non-integrated assembler in synch for Linux like Android, even at the risk of adding an fpu when it isn't needed. I think that given how difficult it is to not use NEON, I think the mfloat-abi=s

[PATCH] D64415: Consistent types and naming for AArch64 target features (NFC)

peter.smith added a comment. FWIW I think this looks reasonable. The ARM equivalent uses bitfields such as unsigned CRC : 1; unsigned Crypto : 1; unsigned DSP : 1; unsigned Unaligned : 1; unsigned DotProd : 1; Which would make more sense than using unsigned for each individual field.

[PATCH] D65000: [ARM] Set default alignment to 64bits

peter.smith added reviewers: srhines, danalbert. peter.smith added a comment. I think that this may not apply for Android as AFAIK their ABI still requires 128-bit alignment in some cases. Adding some more reviewers from Android. Comment at: lib/Basic/Targets/ARM.cpp:311

[PATCH] D65000: [ARM] Set default alignment to 64bits

peter.smith added a comment. Thanks for the update. Will be worth adding some reviewers from Apple to see if this change should be IsAAPCS only. I've no more further comments myself besides a small nit on style. Comment at: lib/Basic/Targets/ARM.cpp:331 + // but Android

[PATCH] D65000: [ARM] Set default alignment to 64bits

peter.smith added a comment. test case missing A8 aside this looks ok to me. Would like to see if there are any comments from the Pacific time zone. Comment at: test/CodeGen/ARM/exception-alignment.cpp:8 +// A16-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 16 +#include +

[PATCH] D65000: [ARM] Set default alignment to 64bits

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've not seen any objections so I've approved LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65000/new/ https://reviews.llvm.org/D65000 _

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

peter.smith added a comment. After retesting on a failing example and experimenting a bit, I think that we should only preserve +crypto with -fno-integrated-as. It would also be good to mention D66018 and maybe add the original author as a reviewer.

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. Thanks for the update. That looks good to me. Will be good to wait for a day before committing to give US timezone a chance to object. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D80828: [Clang][A32/T32][Linux] -O2 implies -fomit-frame-pointer

psmith added a comment. FWIW I'm happy for Clang to match GCC behaviour for Linux (non-Android) targets with respect to the frame pointer. Outside of Android I would expect most developers of linux applications to build with both GCC and clang so they should already have the -fno-omit-frame-poi

[PATCH] D80828: [Clang][A32/T32][Linux] -O1 implies -fomit-frame-pointer

psmith accepted this revision. psmith added a comment. LGTM from an Arm person now that the Android changes have been made. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:22 #include "InputInfo.h" +#include "MSP430.h" #include "PS4CPU.h" nickdesaulniers wr

[PATCH] D78511: [Driver][doc] Document option -mtune as a no-op. NFC.

psmith accepted this revision. psmith added a comment. This revision is now accepted and ready to land. That wording looks good to me. I've accepted the change, but worth waiting a day or so to see if there are any objections or suggestions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D78565: [clang][doc] Clang ARM CPU command line argument reference

psmith added a comment. One question I can't answer and I think would need wider review, is whether this is type of material (common options for specific CPUs) is suited for the Clang Documentation or whether it would be better hosted by Arm itself, for example on developer.arm.com? I think tha

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith created this revision. psmith added reviewers: jfb, Shayne, emaste, kristof.beyls, mattdr, ojhunt, probinson, reames, serge-sans-paille, dim. Herald added a subscriber: dexonsmith. psmith requested review of this revision. The Clang -fstack-protector documentation mentions what functions a

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith added a comment. In D85239#2194604 , @MaskRay wrote: > This file is auto generated. The real documentation should go to > `clang/include/clang/Driver/Options.td` Thanks for pointing that out! I regenerated the ClangCommandLineReference.rst from O

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith updated this revision to Diff 283201. psmith added a comment. Herald added a subscriber: dang. uploaded diff with both Options.td and ClangCommandLineReference.rst CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85239/new/ https://reviews.llvm.org/D85239 Files: clang/docs/ClangC

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith marked an inline comment as done. psmith added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2139 -Enable stack protectors for some functions vulnerable to stack smashing. This uses a loose heuristic which considers functions vulnerable if they c

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

This revision was automatically updated to reflect the committed changes. psmith marked an inline comment as done. Closed by commit rG839d974ee0e4: [DOCS] Add more detail to stack protector documentation (authored by psmith). Herald added a project: clang. Repository: rG LLVM Github Monorepo C

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

psmith added a comment. I agree it's not what I'd like. I'm not sure how to react to MaskRays comment. The top of the ClangCommandLineReference.rst says: --- NOTE: This file is automatically generated by running clang-tblgen -

[PATCH] D133109: [LLVM][ARM] Remove options for armv2, 2A, 3 and 3M

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM. GCC no longer supports Arm architecture prior to v4 so it is likely the alternative of adding support for v3 is not worth it. The only Arm machines running v2 are likely to be

[PATCH] D71688: [AArch64] Add -mtls-size option for ELF targets

This revision was automatically updated to reflect the committed changes. Closed by commit rG10c11e4e2d05: This option allows selecting the TLS size in the local exec TLS model, which is… (authored by kawashima-fj, committed by peter.smith). Herald added a subscriber: cfe-commits. Repository:

[PATCH] D71688: [AArch64] Add -mtls-size option for ELF targets

peter.smith added a comment. Committed as 10c11e4e2d05cf0e8f8251f50d84ce77eb1e9b8d , I've used the new attribution process https://llvm.org/docs/DeveloperPolicy.html so you should show up as the author of the patch in the com

[PATCH] D91760: [Driver] Default Generic_GCC aarch64 to use -fasynchronous-unwind-tables

psmith added a comment. Personally I'm in favour of clang and gcc behaving the same way unless there is a good reason not to. I've shared the review internally to see if anyone has any concerns. May be worth informing the clang built linux community to see if they will need to make any alterati

[PATCH] D91760: [Driver] Default Generic_GCC aarch64 to use -fasynchronous-unwind-tables

psmith added a comment. Radio silence so far; I think no news is good news applies in this case. I'm happy to say no objections. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91760/new/ https://reviews.llvm.org/D91760

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

peter.smith added a comment. Doing this on 32-bit Arm would make me nervous as STT_FUNC symbols encode the state of Arm/Thumb in the bottom bit, but STT_NOTYPE symbols do not. In principle it could be done but extra care would have to be taken to make sure no state changes were required. For ex

[PATCH] D112600: [ARM] Use hardware TLS register in Thumb2 mode when -mtp=cp15 is passed

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM as this as CP15 can be used on architecture v6k and above, which maps to IsThumb2. As an aside from this patch, the Arm state could be considered too permissive as it will perm

[PATCH] D112600: [ARM] Use hardware TLS register in Thumb2 mode when -mtp=cp15 is passed

peter.smith added inline comments. Comment at: clang/test/CodeGen/arm-tphard.c:10 +} + nickdesaulniers wrote: > ardb wrote: > > nickdesaulniers wrote: > > > Let's make this a test under llvm/test/CodeGen/, using IR: > > > ``` > > > ; RUN: llc --mtriple=armv7-linu

[PATCH] D112768: [ARM] implement support for TLS register based stack protector

peter.smith added a reviewer: ostannard. peter.smith added a comment. Adding ostannard to reviewers list. I'm going to be on vacation next week and Oliver is more familiar with this area than I am. To prevent the option in Clang for targets that don't support Thumb2 it may be worth looking into

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for ARMv6 non-thumb cases

peter.smith added a comment. Apologies had to go on a bit of a dive through the documentation. I've put some comments inline and some links to other documents that may help. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:150 +// The backend does not have support for ha

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for ARMv6 non-thumb cases

peter.smith added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155 + llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName()); + return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 || + (Ver == 6 && Triple.isARM());

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

peter.smith added a comment. I've made a suggestion to disallow v8-m.baseline (does not have Thumb 2 but has number > 7) and to simplify the expression. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155 + llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

peter.smith added a comment. Apologies, missed a couple of small things out. Otherwise looks good to me. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:150 +// The backend does not have support for hard thread pointers when targeting +// Thumb1. peter.s

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. LGTM, thanks for the update. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114116/new/ https://reviews.llvm.org/D114116 _

[PATCH] D114124: [clang][ARM] only check -mtp=cp15 for non-asm sources

peter.smith added a comment. LGTM too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114124/new/ https://reviews.llvm.org/D114124 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llv

[PATCH] D136385: Add support for MC/DC in LLVM Source-Based Code Coverage

peter.smith added a comment. I've got about as far as the clang changes. As I mentioned in Discourse I'm not familiar enough in this area to give good feedback on the implementation, most if not all my comments fall under the bike shedding category. Will need to leave the high-level feedback to

[PATCH] D142893: [NFC] Class for building MultilibSet

peter.smith added a comment. Looks good so far. Some stylistic suggestions. Comment at: clang/include/clang/Driver/Multilib.h:25 namespace driver { /// This corresponds to a single GCC Multilib, or a segment of one controlled It took a bit of reading to wor

[PATCH] D142905: Change multilib selection algorithm

peter.smith added a comment. A couple of small stylisitic suggestions. Comment at: clang/include/clang/Driver/Multilib.h:31 public: - using flags_list = std::vector; + using flags_list = std::set; + using arg_list = std::vector; would flags_set be a better

[PATCH] D142933: Add -print-multi-selection-flags argument

peter.smith added inline comments. Comment at: clang/include/clang/Driver/Options.td:4162 def print_multi_lib : Flag<["-", "--"], "print-multi-lib">; +def print_multi_selection_flags : Flag<["-", "--"], "print-multi-selection-flags-experimental">, + HelpText<"Print the flags u

[PATCH] D155808: [clang][driver] Missing the condition in IsARMBigEndain function.

peter.smith added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:39 // normalized triple so we must handle the flag here. bool arm::isARMBigEndian(const llvm::Triple &Triple, const ArgList &Args) { + if ((Triple.getArch() == llvm::Triple::arm ||

[PATCH] D73865: [CodeGenModule] Assume dso_local for -fpic -fno-semantic-interposition

peter.smith added a comment. If I've understood correctly this would make LLVM more aggressive for PIC relocation models, but perhaps more honest in an inter procedural optimisation context? The code changes look fine to me, I'm wondering if we've discussed this widely enough with the community

[PATCH] D73904: [clang] stop baremetal driver to append .a to lib

peter.smith added a comment. I think this is the right thing to do. According to https://sourceware.org/binutils/docs/ld/Options.html#Options Add the archive or object file specified by namespec to the list of files to link. This option may be used any number of times. If namespec is of the f

[PATCH] D73904: [clang] stop baremetal driver to append .a to lib

psmith accepted this revision. psmith added a comment. This revision is now accepted and ready to land. Thanks for the update, looks good to me. The BareMetal driver tests are better than the location I suggested. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73904/new/ https://reviews

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

peter.smith added a comment. It is also failing on the other 32-bit arm bots, example http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/13052/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Avirtual-compare.cpp Looking at the failure // CHECK-SAME: @_ZThn8_N1AD1Ev and the out

[PATCH] D72959: Relative VTables ABI on Fuchsia

peter.smith added a comment. In D72959#1832011 , @pcc wrote: > > On Aarch64, right now only CALL26 and JUMP26 instructions generate PLT > > relocations, so we manifest them with stubs that are just jumps to the > > original function. > > I think it would

[PATCH] D72959: Relative VTables ABI on Fuchsia

peter.smith added a comment. In D72959#1835368 , @rjmccall wrote: > There's two independently-useful things here for the relocation, right? > First, it's useful to be able to express a relocation to a symbol that has > the semantics of a particular func

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

peter.smith added a comment. Although this particular commit will not be at fault, it is the option that enables the feature which is the earliest I can bisect the fault to. There are 3 files in linux that assert fail on the Implement the 'patchable-function attribute'. The files in question ar

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

peter.smith added a comment. >> If the patchable functions is intended for clang-10 we'll need to make sure >> any fix is merged to clang-10. > > This commit was made before release/10.x branch. Maybe the easiest thing is > to revert the driver change in release/10.x (CC @hans), before we had a

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

peter.smith added a comment. >> @peter.smith @nickdesaulniers What do you think? > > Revert on the 10.0 release sounds reasonable to me. That would prevent the > kernel from enabling the option and would prevent the build failure. I should have been clearer, apologies; we're not blocked by the

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

peter.smith added a comment. In D7#1836703 , @MaskRay wrote: > In D7#1836669 , @hans wrote: > > > In D7#1836643 , @peter.smith > > wrote: > > > > > >> If the pat

[PATCH] D44815: [AArch64]: Add support for parsing rN registers.

peter.smith added a comment. I think that we may have a bit of a conceptual difference with GCC here. As far as I can tell in GCC it doesn't matter if "rn", "wn", "xn" or even "bn" is used, this refers to register 0. The Operand constraint such as %w0 is used to control the substitution so the

[PATCH] D44815: [AArch64]: Add support for parsing rN registers.

peter.smith added a comment. Thanks for the clarification. With that in mind I'm much less concerned that adding "r" as an alias will make extra warnings more difficult. I agree that there should be at least a warning for register long x asm ("wn") although that is separate from this patch. Ha

[PATCH] D44815: [AArch64]: Add support for parsing rN registers.

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. Given that this is important for compiling the Linux kernel with clang I think that it is worth improving compatibility with GCC. LGTM. Repository: rC Clang https://reviews.llvm.

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

peter.smith created this revision. Herald added subscribers: kristof.beyls, javed.absar, aemerson. The Unified Arm Assembler Language is designed so that the majority of assembler files can be assembled for both Arm and Thumb with the choice made as a compilation option. The way this is done in

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

peter.smith added inline comments. Comment at: lib/Driver/ToolChain.cpp:549-556 +bool IsIntegratedAssemblerThumb = false; +for (const Arg *A : + Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) { + for (StringRef Value : A->getValues()) { +

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

peter.smith updated this revision to Diff 123310. peter.smith added a comment. Updated diff with an attempt to simplify the check for filetype and mthumb. I've left the existing Args.filtered in expression for now as I couldn't make a better alternative with std::for_any. https://reviews.llvm.

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

This revision was automatically updated to reflect the committed changes. Closed by commit rL318647: [ARM] For assembler files recognize -Xassembler or -Wa, -mthumb (authored by psmith). Changed prior to commit: https://reviews.llvm.org/D40127?vs=123310&id=123571#toc Repository: rL LLVM htt

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

peter.smith added a comment. In https://reviews.llvm.org/D40127#929578, @compnerd wrote: > Would be nice to rename the variable prior to commit. Thanks for the review, I've renamed the variable as suggested. https://reviews.llvm.org/D40127 ___ cf

[PATCH] D142905: [Driver] Change multilib selection algorithm

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left a suggestion for a comment, bu

[PATCH] D145567: [Driver] Rename multilib flags to tags

peter.smith added a comment. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. From looking at the source code alone - assuming that most people would not track down the commit message/review for extra c

[PATCH] D145567: [Driver] Rename multilib flags to tags

peter.smith accepted this revision. peter.smith added a comment. Actually click the button this time to set approval, see previous comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145567/new/ https://reviews.llvm.org/D145567

[PATCH] D142932: Multilib YAML parsing

peter.smith added a comment. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left some comments that can be applied prior to commit if you want to take them up. Comment at: clan

[PATCH] D142933: Add -print-multi-selection-tags-experimental option

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. Repository: rG LLVM Github Monorepo

[PATCH] D142986: Enable multilib.yaml in the BareMetal ToolChain

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. Repository: rG LLVM Github Monorepo

[PATCH] D143059: [Driver] Enable selecting multiple multilibs

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. Repository: rG LLVM Github Monorepo

[PATCH] D143075: BareMetal ToolChain multilib layering

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. Repository: rG LLVM Github Monorepo

[PATCH] D143587: [Docs] Multilib design

peter.smith accepted this revision. peter.smith added a comment. This revision is now accepted and ready to land. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left a suggestion that can be appli

  1   2   >