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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
