[PATCH] D103943: [X86] Add -mgeneral-regs-only support.

[Wang Tianqing via Phabricator via cfe-commits]

tianqing marked an inline comment as done.
tianqing added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:3214
 
-def mgeneral_regs_only : Flag<["-"], "mgeneral-regs-only">, Group<m_aarch64_Features_Group>,
-  HelpText<"Generate code which only uses the general purpose registers (AArch64 only)">;
+def mgeneral_regs_only : Flag<["-"], "mgeneral-regs-only">, Group<m_Group>,
+  HelpText<"Generate code which only uses the general purpose registers (AArch64/x86 only)">;
----------------
pengfei wrote:
> Will this change affect AArch64 or other targets expect AArch64 and x86?
No, using this option on other targets gives "argument unused during compilation" warning.


================
Comment at: clang/lib/Basic/Targets/X86.cpp:120
 
-  if (!TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec))
+  std::vector<std::string> UpdatedFeaturesVec;
+  for (const auto& Feature : FeaturesVec) {
----------------
pengfei wrote:
> Why do we need to expand it again after we handling in driver?
The driver handles command line options, and this handles "target" attributes.
Just adding "+general-regs-only" in the driver also works. But it's not in OPT_m_x86_Features_Group, and current handleTargetFeaturesGroup will ignore it so we still have to copy its code, not much of a difference.

(Note AArch64 only supports options)


================
Comment at: clang/lib/Basic/Targets/X86.cpp:136-158
   // Can't do this earlier because we need to be able to explicitly enable
   // or disable these features and the things that they depend upon.
 
   // Enable popcnt if sse4.2 is enabled and popcnt is not explicitly disabled.
   auto I = Features.find("sse4.2");
   if (I != Features.end() && I->getValue() &&
+      llvm::find(UpdatedFeaturesVec, "-popcnt") == UpdatedFeaturesVec.end())
----------------
pengfei wrote:
> Shouldn't this be simply skipped under "general-regs-only"?
It's still about order of options. Seeing a "general-regs-only" doesn't mean the function is really GPR only, we have to apply all options in order and check the result.


================
Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:216-235
+  for (const Arg *A : Args.filtered(options::OPT_m_x86_Features_Group,
+                                    options::OPT_mgeneral_regs_only)) {
+    StringRef Name = A->getOption().getName();
+    A->claim();
+
+    // Skip over "-m".
+    assert(Name.startswith("m") && "Invalid feature name.");
----------------
pengfei wrote:
> Why we need copy the code here? Can it be simply use:
> ```
> if (Args.getLastArg(options::OPT_mgeneral_regs_only))
>   Features.insert(Features.end(), {"-x87", "-mmx", "-sse"});
> handleTargetFeaturesGroup(Args, Features, options::OPT_m_x86_Features_Group);
> ```
To make sure later options override earlier ones. This is how GCC behaves.

This is demonstrated in the "GPR" and "AVX2" check lines of the new tests.


================
Comment at: clang/test/CodeGen/attr-target-general-regs-only-x86.c:14
+// CHECK: attributes [[GPR_ATTRS]] = { {{.*}} "target-features"="{{.*}}-avx{{.*}}-avx2{{.*}}-avx512f{{.*}}-sse{{.*}}-sse2{{.*}}-ssse3{{.*}}-x87{{.*}}"
+// CHECK: attributes [[AVX2_ATTRS]] = { {{.*}} "target-features"="{{.*}}+avx{{.*}}+avx2{{.*}}+sse{{.*}}+sse2{{.*}}+ssse3{{.*}}-avx512f{{.*}}-x87{{.*}}"
----------------
pengfei wrote:
> Why we have a `-avx512f` when enabling `avx2`?
See llvm::X86::updateImpliedFeatures(), enabling a feature will enable all features it depends on, disabling a feature also disables all features depending on it. This is "target feature inheritance" mentioned in Simon's comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103943/new/

https://reviews.llvm.org/D103943