kristof.beyls added inline comments.
================ Comment at: include/clang/Driver/Options.td:1664-1665 HelpText<"Allow generation of data access to code sections (ARM only)">; +def mtp_mode_EQ : Joined<["-"], "mtp=">, Group<m_arm_Features_Group>, Values<"soft, cp15">, + HelpText<"Read thread pointer from coprocessor register (ARM only)">; def mpure_code : Flag<["-"], "mpure-code">, Alias<mexecute_only>; // Alias for GCC compatibility ---------------- Looking at the gcc documentation for this option (https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html), gcc accepts 3 values: 'soft', 'cp15' and 'auto', with the default setting being 'auto'. This patch implements just 2 of those values: 'soft' and 'cp15'. I think this is fine, as long as the default value is 'soft'. The 'auto' value should automatically pick 'cp15' if that's going to work on what you're targeting. If I understood correctly, that depends both on the architecture version you're targeting and the operating system/kernel you're targeting. So, there could be a lot of details to go through to get 'auto' right in all cases. Which is why I think it's fine to leave an implementation of 'auto' for later. Is the default value 'soft'? ================ Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:128 + const Driver &D = TC.getDriver(); + arm::ReadTPMode ThreadPointer = ReadTPMode::Invalid; + if (Arg *A = ---------------- Wouldn't it be better to default to ReadTPMode::Soft when not -mtp command line option is given? .... ================ Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:145-146 + // choose soft mode. + if (ThreadPointer == ReadTPMode::Invalid) + ThreadPointer = ReadTPMode::Soft; + return ThreadPointer; ---------------- .... and always give an error if an invalid mtp command line option was given, rather than default back to soft mode? ================ Comment at: lib/Driver/ToolChains/Clang.cpp:1348-1358 + arm::ReadTPMode ThreadPointer = arm::getReadTPMode(getToolChain(), Args); + if (ThreadPointer == arm::ReadTPMode::Cp15) { + CmdArgs.push_back("-mtp"); + CmdArgs.push_back("cp15"); + } else { + assert(ThreadPointer == arm::ReadTPMode::Soft && + "Invalid mode for reading thread pointer"); ---------------- My inexperience in this part of the code base is probably showing, but why is this needed at all? IIUC, in D34408, you modelled TPMode in the backend using a target feature, and there isn't a custom -mtp option there? Maybe this is left-over code from an earlier revision of D34408, that's no longer needed? ================ Comment at: test/Driver/clang-translation.c:78-82 +// RUN: %clang -target arm-linux -mtp=cp15 -### -S %s -arch armv7 2>&1 | \ +// RUN: FileCheck -check-prefix=ARMv7_THREAD_POINTER %s +// ARMv7_THREAD_POINTER: "-target-feature" "+read-tp-hard" +// ARMv7_THREAD_POINTER: "-mtp" "cp15" +// ARMv7_THREAD_POINTER-NOT: "mtp" "soft" ---------------- It probably would be good to also have a test that when no mtp option is given, the equivalent of when '-mtp soft' is specified would happen. Furthermore, my inexperience in this part of the code base probably shows, but I'm puzzled as to why this test is looking for '-mtp' in the output. Wouldn't the '-target-feature +read-tp-hard' be enough to convey the information to the mid- and back-end? Repository: rL LLVM https://reviews.llvm.org/D34878 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits