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