spetrovic marked 3 inline comments as done. spetrovic 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 ---------------- kristof.beyls wrote: > 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'? I agree with your opinion about 'auto'. If -mtp option is not specified, yes, default value is soft. ================ Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:128 + const Driver &D = TC.getDriver(); + arm::ReadTPMode ThreadPointer = ReadTPMode::Invalid; + if (Arg *A = ---------------- kristof.beyls wrote: > Wouldn't it be better to default to ReadTPMode::Soft when not -mtp command > line option is given? .... When there is no -mtp in command line ReadTPMode::Soft is default value, ReadTPMode::Invalid is in case when someone try to put in -mtp value that is not cp15 or soft (e.g. -mtp=x). ================ Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:145-146 + // choose soft mode. + if (ThreadPointer == ReadTPMode::Invalid) + ThreadPointer = ReadTPMode::Soft; + return ThreadPointer; ---------------- kristof.beyls wrote: > .... and always give an error if an invalid mtp command line option was > given, rather than default back to soft mode? If 'mtp' takes invalid value, error is always provided. This is the case when there is no -mtp option in command line, you can see how the case of invalid mtp argument is handled in the code above. ================ 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"); ---------------- kristof.beyls wrote: > 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? Well, actually you are right, we can remove this part of the code, I was thinking that maybe someone in future will need in backend that '-mtp' option also can be recgonized, so I added this, but you are right, I will remove this. ================ 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" ---------------- kristof.beyls wrote: > 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? Checks for '-mtp' in the output are unnecessary when we remove part of the code that you mentioned in previous comment. 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
