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

[Pengfei Wang via Phabricator via cfe-commits]

pengfei 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)">;
----------------
Will this change affect AArch64 or other targets expect AArch64 and x86?


================
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) {
----------------
Why do we need to expand it again after we handling in driver?


================
Comment at: clang/lib/Basic/Targets/X86.cpp:121
+  std::vector<std::string> UpdatedFeaturesVec;
+  for (const auto& Feature : FeaturesVec) {
+    // Expand general-regs-only to -x86, -mmx and -sse
----------------
Please follow Lint's comment.


================
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())
----------------
Shouldn't this be simply skipped under "general-regs-only"?


================
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.");
----------------
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);
```


================
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{{.*}}"
----------------
Why we have a `-avx512f` when enabling `avx2`?


================
Comment at: clang/test/Driver/x86-mgeneral-regs-only.c:26
+// IR-GPR: attributes {{.*}} = { {{.*}} "target-features"="{{.*}}-avx{{.*}}-avx2{{.*}}-avx512f{{.*}}-sse{{.*}}-sse2{{.*}}-ssse3{{.*}}-x87{{.*}}"
+// IR-AVX2: attributes {{.*}} = { {{.*}} "target-features"="{{.*}}+avx{{.*}}+avx2{{.*}}+sse{{.*}}+sse2{{.*}}+ssse3{{.*}}-avx512f{{.*}}-x87{{.*}}"
----------------
ditto


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

https://reviews.llvm.org/D103943