[X86] Use generic CPU tuning when tune-cpu is empty

[AtariDreams]

To prevent the test results from being bugged, I had to edit the initializer of the vectorizer loop to ensure the command line parameters were heeded, and then updated the tests.

[llvmbot]

@llvm/pr-subscribers-lld-coff
@llvm/pr-subscribers-lld-macho
@llvm/pr-subscribers-lld
@llvm/pr-subscribers-llvm-globalisel
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-backend-x86

Author: AtariDreams (AtariDreams)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/83631.diff

1 Files Affected:

• (modified) llvm/lib/Target/X86/X86Subtarget.cpp (+1-1)

diff --git a/llvm/lib/Target/X86/X86Subtarget.cpp b/llvm/lib/Target/X86/X86Subtarget.cpp
index 07f535685e8f97..0c263a2e9c3dd8 100644
--- a/llvm/lib/Target/X86/X86Subtarget.cpp
+++ b/llvm/lib/Target/X86/X86Subtarget.cpp
@@ -252,7 +252,7 @@ void X86Subtarget::initSubtargetFeatures(StringRef CPU, StringRef TuneCPU,
     CPU = "generic";
 
   if (TuneCPU.empty())
-    TuneCPU = "i586"; // FIXME: "generic" is more modern than llc tests expect.
+    TuneCPU = "generic";
 
   std::string FullFS = X86_MC::ParseX86Triple(TargetTriple);
   assert(!FullFS.empty() && "Failed to parse X86 triple");

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

[topperc]

This doesn't change any tests?

[AtariDreams]

This doesn't change any tests?

It does now.

[RKSimon]

Please check the CI - these failures look relevant

Failed Tests (4):
  lld :: COFF/lto-cpu-string.ll
  lld :: COFF/lto.ll
  lld :: ELF/lto/cpu-string.ll
  lld :: MachO/lto-cpu-string.ll

[AtariDreams]

Please check the CI - these failures look relevant

Failed Tests (4):
  lld :: COFF/lto-cpu-string.ll
  lld :: COFF/lto.ll
  lld :: ELF/lto/cpu-string.ll
  lld :: MachO/lto-cpu-string.ll

they are and I am getting to them.

[phoebewang]

This is a turbulent change to both upstream and downstream tests without any profit as far as I can tell.

I did a similar change for 64-bit a few years ago: https://reviews.llvm.org/D129647

In comparison, this patch is not to solve a specific problem. It should not show any value because we don't care about 32-bit performance. Not to mention, we need to keep the test not changed as many as possible. The way we used in D129647 is to add an explicit "tune-cpu". We cannot blindly update tests in case distort the original intention.

[topperc]

This is a turbulent change to both upstream and downstream tests without any profit as far as I can tell.

I did a similar change for 64-bit a few years ago: https://reviews.llvm.org/D129647

In comparison, this patch is not to solve a specific problem. It should not show any value because we don't care about 32-bit performance. Not to mention, we need to keep the test not changed as many as possible. The way we used in D129647 is to add an explicit "tune-cpu". We cannot blindly update tests in case distort the original intention.

I think I probaby wrote the FIXME and I agree with @phoebewang

[AtariDreams]

This is a turbulent change to both upstream and downstream tests without any profit as far as I can tell.

I did a similar change for 64-bit a few years ago: https://reviews.llvm.org/D129647

In comparison, this patch is not to solve a specific problem. It should not show any value because we don't care about 32-bit performance. Not to mention, we need to keep the test not changed as many as possible. The way we used in D129647 is to add an explicit "tune-cpu". We cannot blindly update tests in case distort the original intention.

I think I probaby wrote the FIXME and I agree with @phoebewang

Wait does this only affect 32 bit?

[phoebewang]

This is a turbulent change to both upstream and downstream tests without any profit as far as I can tell.

I did a similar change for 64-bit a few years ago: https://reviews.llvm.org/D129647

In comparison, this patch is not to solve a specific problem. It should not show any value because we don't care about 32-bit performance. Not to mention, we need to keep the test not changed as many as possible. The way we used in D129647 is to add an explicit "tune-cpu". We cannot blindly update tests in case distort the original intention.

I think I probaby wrote the FIXME and I agree with @phoebewang

Wait does this only affect 32 bit?

No, I took another look. It not only affects 32 bit. The 64 bit uses i586 tuning as well, which should be fixed. So there would be still a lot of tests need to update.

[AtariDreams]

@phoebewang What do you think?

[phoebewang]

@phoebewang What do you think?

This happens only when both CPU and TuneCPU are empty. Front ends like Clang always sets a valid CPU, while most llc tests are not sensitive to tuning targets (the rest already set target-cpu/tune-cpu). So I think the change doesn't have much value in reality.

Nevertheless, I think it's good to reduce one FIXME, so LGTM.

[topperc]

This did not fix the FIXME. Checking Is64Bit is the correct fix. Checking HasX86_64 is useless, that's never set if there is no CPU provided. HasX86_64 just means the CPU is a 64-bit capable CPU, not that 64-bit mode is enabled

[phoebewang]

This did not fix the FIXME. Checking Is64Bit is the correct fix. Checking HasX86_64 is useless, that's never set if there is no CPU provided. HasX86_64 just means the CPU is a 64-bit capable CPU, not that 64-bit mode is enabled

Oh, right. The generic CPU will set HasX86_64 too.

[phoebewang]

Reverted it.

[topperc]

Even Is64Bit won't work because we haven't called ParseSubtargetFeatures yet, so no flags are set. The best we could do is inspect the arch from the Triple to detect 64-bit.