[X86] Introduce more common modern tunings into `generic`

[pengfei]

Summary

GCC has updated its generic -mtune to haswell. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81616
Update it to match with GCC.

[RKSimon]

llvm/lib/Target/X86/X86.td

1508

The old tuning flags didn't match all of SandyBridge's - do we really want to match all of Haswell's?

[RKSimon]

Also, I'm slightly worried about this being dropped in just before the 14.x branch - would it be better to ensure we miss 14.x so this has time to cook in trunk for a while?

[pengfei]

In D118534#3281634, @RKSimon wrote:

Also, I'm slightly worried about this being dropped in just before the 14.x branch - would it be better to ensure we miss 14.x so this has time to cook in trunk for a while?

Sure, I can wait for 14.x freeze. Thanks!

llvm/lib/Target/X86/X86.td

1508

The GCC doc says generic will be updated with compiler iteration. I think it's reasonable since the common processors will be upgraded with time.

[spatel]

The default tuning came up in the context of bsf vs. tzcnt here:
D117912
So I wasn't expecting a bump in default tuning yet, but I think it's great.

But I agree that this change may cause a lot of surprises, so users need to be given notice (add a line to the clang release notes?) and wait for the 14.0 branch to be created.

Should we have a test file that intentionally shows codegen changes as this setting gets updated?

[lebedev.ri]

In D118534#3281771, @spatel wrote:

The default tuning came up in the context of bsf vs. tzcnt here:
D117912
So I wasn't expecting a bump in default tuning yet, but I think it's great.

But I agree that this change may cause a lot of surprises, so users need to be given notice (add a line to the clang release notes?) and wait for the 14.0 branch to be created.

Should we have a test file that intentionally shows codegen changes as this setting gets updated?

Please note that there is a *very* important difference between -march= and -mtune=:

• -march= means: no, please, i really only intend to run this code on this specific CPU, so feel free to use all of the ISA available, and schedule for this cCPU too.
• while -mtune= means: i want to be able to run the code on earlier processors, but do schedule for this CPU though.

So this change really shouldn't affect available instruction sets.

[spatel]

In D118534#3281811, @lebedev.ri wrote:

In D118534#3281771, @spatel wrote:

The default tuning came up in the context of bsf vs. tzcnt here:
D117912
So I wasn't expecting a bump in default tuning yet, but I think it's great.

But I agree that this change may cause a lot of surprises, so users need to be given notice (add a line to the clang release notes?) and wait for the 14.0 branch to be created.

Should we have a test file that intentionally shows codegen changes as this setting gets updated?

Please note that there is a *very* important difference between -march= and -mtune=:

• -march= means: no, please, i really only intend to run this code on this specific CPU, so feel free to use all of the ISA available, and schedule for this cCPU too.
• while -mtune= means: i want to be able to run the code on earlier processors, but do schedule for this CPU though.

So this change really shouldn't affect available instruction sets.

Ah, thanks for explaining. I confused the settings. So it's still a small bit of progress, just not as much as I thought.

[craig.topper]

llvm/lib/Target/X86/X86.td

1508

This turns in some fast shuffle flags that aren’t used by any AMD CPU except znver3. Do those flags make sense for generic?

[pengfei]

In D118534#3281820, @spatel wrote:

In D118534#3281811, @lebedev.ri wrote:

In D118534#3281771, @spatel wrote:

The default tuning came up in the context of bsf vs. tzcnt here:
D117912
So I wasn't expecting a bump in default tuning yet, but I think it's great.

I must have missed the comments there, but seems we made the consensus independently.

But I agree that this change may cause a lot of surprises, so users need to be given notice (add a line to the clang release notes?) and wait for the 14.0 branch to be created.

Should we have a test file that intentionally shows codegen changes as this setting gets updated?

Please note that there is a *very* important difference between -march= and -mtune=:

• -march= means: no, please, i really only intend to run this code on this specific CPU, so feel free to use all of the ISA available, and schedule for this cCPU too.
• while -mtune= means: i want to be able to run the code on earlier processors, but do schedule for this CPU though.

So this change really shouldn't affect available instruction sets.

Yes. Thanks @lebedev.ri

Ah, thanks for explanation. I confused the settings. So it's still a small bit of progress, just not as much as I thought.

As I read the comments in D117912, the changes in default tuning should solve the problem. Do I miss something else?
I saw some attempts on using x86-64-v2, e.g., RHEL 9. But I think it's still aggressive to use it as default target in compiler, not to mention x86-64-v3.

[pengfei]

llvm/lib/Target/X86/X86.td

1508

I saw someone checked on znver1 and gave a conclusion the haswell tuning works reasonably well for both cores and zens. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81616
I have no idea how to check it with LLVM, so I'll remove the shuffle flags for conservative.

[pengfei]

Remove shuffle flags.

[spatel]

In D118534#3282197, @pengfei wrote:

As I read the comments in D117912, the changes in default tuning should solve the problem. Do I miss something else?
I saw some attempts on using x86-64-v2, e.g., RHEL 9. But I think it's still aggressive to use it as default target in compiler, not to mention x86-64-v3.

I might have misunderstood the requirements, but I thought we won't get the ideal performance on the benchmark unless we can generate tzcnt without explicitly specifying the default CPU arch.

[lebedev.ri]

In D118534#3282807, @spatel wrote:

In D118534#3282197, @pengfei wrote:

As I read the comments in D117912, the changes in default tuning should solve the problem. Do I miss something else?
I saw some attempts on using x86-64-v2, e.g., RHEL 9. But I think it's still aggressive to use it as default target in compiler, not to mention x86-64-v3.

I might have misunderstood the requirements, but I thought we won't get the ideal performance on the benchmark unless we can generate tzcnt without explicitly specifying the default CPU arch.

That would require bumping what ISA's are implied by generic, yes.

[pengfei]

In D118534#3282808, @lebedev.ri wrote:

In D118534#3282807, @spatel wrote:

In D118534#3282197, @pengfei wrote:

As I read the comments in D117912, the changes in default tuning should solve the problem. Do I miss something else?
I saw some attempts on using x86-64-v2, e.g., RHEL 9. But I think it's still aggressive to use it as default target in compiler, not to mention x86-64-v3.

I might have misunderstood the requirements, but I thought we won't get the ideal performance on the benchmark unless we can generate tzcnt without explicitly specifying the default CPU arch.

That would require bumping what ISA's are implied by generic, yes.

I see. Yes, we can do nothing with it currently.

[RKSimon]

llvm/lib/Target/X86/X86.td

1518

We now diverge from the x86-64 tuning flags - maybe make it clear in the comments for the generic model?

llvm/test/CodeGen/X86/rdtsc-upgrade.ll

3 ↗

(On Diff #404317)

do we even need to specificy -mcpu here?

llvm/test/CodeGen/X86/rdtsc.ll

3 ↗

(On Diff #404317)

do we really need -mcpu?

llvm/test/CodeGen/X86/twoaddr-lea.ll

2

not sure about -mcpu here - we do have some variation in use of lea depending on tuning - maybe we need more -mcpu coverage?

llvm/test/tools/llvm-mca/X86/cv_fpo_directive_no_segfault.s

2 ↗

(On Diff #404317)

@andreadb what test coverage do we need here?

[andreadb]

llvm/lib/Target/X86/X86.td

1223

In my experience, SHLD is rarely fast on AMD processors.

1225–1226

These two tuning flags are very Intel specific.
I am not convinced that these should be added for "generic".

llvm/test/tools/llvm-mca/X86/cv_fpo_directive_no_segfault.s

2 ↗

(On Diff #404317)

I believe the test was just checking that llvm-mca didn't crash when parsing unknown asm directives.

So the output here is not really important. It is fine to change the mcpu to something else other than generic.

[pengfei]

Remove more Intel specific tunings.

[pengfei]

llvm/test/CodeGen/X86/twoaddr-lea.ll

2

Here the generated code is affected by tuning TuningSlowIncDec.
I think we should prefer to using -mcpu=x86-64 rather than generic in lit tests since we might update the tunings of generic in future.
We have more than 100 lines of RUNs with it, we may need a seperate patch to replace them. But the diff here is OK to reflect the change on tunings.

[pengfei]

Ping

[spatel]

llvm/lib/Target/X86/X86.td

1222

Fast scalar SQRT controls whether we produce a single-precision sqrtss instruction or a reciprocal estimate sequence of about 9 instructions (when allowed with fast-math).

Based on Agner's timing docs and the flag description, this should be set for Zen1 (sqrtss has latency 9-10), but it's not as obviously good for Zen2/3 because those have sqrtss latency of 14. The flag is set for Intel CPUs since SandyBridge, so that's sqrtss latency between 10-14.

I think this is ok to set, but if the assumption is that we're tuning for any mainstream CPU of the last N years, then shouldn't we add this flag to the later AMD models too for less surprising output? There's a possible side benefit that we will produce more accurate results too.

[pengfei]

Add TuningFastScalarFSQRT to Zen1/2/3.

[pengfei]

llvm/lib/Target/X86/X86.td

1222

uops shows all Zen1/2/3 have the same max latency 14.

[lebedev.ri]

Please split Znver changes into a separate review.
At least for znver3, i'm not really confident that fsqrt is fast,
https://www.agner.org/optimize/instruction_tables.pdf says ~25cy,
while NR takes ~19cy: https://godbolt.org/z/rK9ra4hse

[pengfei]

Revert the change of TuningFastScalarFSQRT for Zen.

[pengfei]

In D118534#3295994, @lebedev.ri wrote:

Please split Znver changes into a separate review.
At least for znver3, i'm not really confident that fsqrt is fast,
https://www.agner.org/optimize/instruction_tables.pdf says ~25cy,
while NR takes ~19cy: https://godbolt.org/z/rK9ra4hse

Although Agner's table says it's 8~21 and 22 for znver1 and znver2 respectively, the mca shows they are worse than znver3. Is it a bug in schedule model? I'd like to leave Znver tuning as is given I'm not familiar with them.

[lebedev.ri]

In D118534#3296058, @pengfei wrote:

In D118534#3295994, @lebedev.ri wrote:

Please split Znver changes into a separate review.
At least for znver3, i'm not really confident that fsqrt is fast,
https://www.agner.org/optimize/instruction_tables.pdf says ~25cy,
while NR takes ~19cy: https://godbolt.org/z/rK9ra4hse

Although Agner's table says it's 8~21 and 22 for znver1 and znver2 respectively, the mca shows they are worse than znver3. Is it a bug in schedule model? I'd like to leave Znver tuning as is given I'm not familiar with them.

znver1/znver2 schedule models are, well, leave a lot to be desired.

[RKSimon]

In D118534#3295994, @lebedev.ri wrote:

Please split Znver changes into a separate review.
At least for znver3, i'm not really confident that fsqrt is fast,
https://www.agner.org/optimize/instruction_tables.pdf says ~25cy,
while NR takes ~19cy: https://godbolt.org/z/rK9ra4hse

'fsqrt' is the x87 instruction, I think the tuning flag (despite its name which is IR based not x87 based) is concerned with the SSE instruction (v)sqrtss - https://godbolt.org/z/qTzesKWvj

But lets make any znver changes independent of this patch

I'm happy for TuningFastScalarFSQRT to be enabled by default - more recent CPUs are benefiting less and less from NR approximations (NR is usually still worth it for float4/float8 though).

[spatel]

In D118534#3296065, @RKSimon wrote:

In D118534#3295994, @lebedev.ri wrote:

Please split Znver changes into a separate review.
At least for znver3, i'm not really confident that fsqrt is fast,
https://www.agner.org/optimize/instruction_tables.pdf says ~25cy,
while NR takes ~19cy: https://godbolt.org/z/rK9ra4hse

'fsqrt' is the x87 instruction, I think the tuning flag (despite its name which is IR based not x87 based) is concerned with the SSE instruction (v)sqrtss - https://godbolt.org/z/qTzesKWvj

Correct. Also AFAIK, that tuning flag only comes into play when expanding a plain sqrt(X) operation, not a 1/sqrt(X) operation. But I agree that we can make that change independently for the zen models.
So this patch LGTM.

[spatel]

In D118534#3296584, @spatel wrote:

Correct. Also AFAIK, that tuning flag only comes into play when expanding a plain sqrt(X) operation, not a 1/sqrt(X) operation. But I agree that we can make that change independently for the zen models.

Proposed change for Zen here: D119001

[pengfei]

Thank you all for the help!

[hans]

I noticed that my builds still prefer "add" over "inc" despite this change. Turns out those builds pass -march=x86-64, which apparently affects the tuning which was a surprise to me. I filed https://github.com/llvm/llvm-project/issues/54472 Does that seem right to the experts on this change?

llvm/lib/Target/X86/X86.td

class ProcModel<string Name, SchedMachineModel Model,
                 list<SubtargetFeature> TuneFeatures>
  : ProcessorModel<Name, Model, Features, TuneFeatures>;
 
 // NOTE: CMPXCHG8B is here for legacy compatibility so that it is only disabled
 // if i386/i486 is specifically requested.
 // NOTE: 64Bit is here as "generic" is the default llc CPU. The X86Subtarget
 // constructor checks that any CPU used in 64-bit mode has Feature64Bit enabled.
 // It has no effect on code generation.
+// NOTE: As a default tuning, "generic" aims to produce code optimized for the
+// most common X86 processors. The tunings might be changed over time. It is
+// recommended to use "x86-64" in lit tests for consistency.
 def : ProcModel<"generic", SandyBridgeModel,
                 [FeatureX87, FeatureCMPXCHG8B, Feature64Bit],
                 [TuningSlow3OpsLEA,
                  TuningSlowDivide64,
-                 TuningSlowIncDec,
                  TuningMacroFusion,
+                 TuningFastScalarFSQRT,
+                 TuningFast15ByteNOP,
                  TuningInsertVZEROUPPER]>;
 
 def : Proc<"i386",            [FeatureX87],
                               [TuningSlowUAMem16, TuningInsertVZEROUPPER]>;
 def : Proc<"i486",            [FeatureX87],
                               [TuningSlowUAMem16, TuningInsertVZEROUPPER]>;
 def : Proc<"i586",            [FeatureX87, FeatureCMPXCHG8B],
                               [TuningSlowUAMem16, TuningInsertVZEROUPPER]>;
 def : Proc<"pentium",         [FeatureX87, FeatureCMPXCHG8B],
                               [TuningSlowUAMem16, TuningInsertVZEROUPPER]>;
 def : Proc<"pentium-mmx",     [FeatureX87, FeatureCMPXCHG8B, FeatureMMX],
def : Proc<"c3-2",            [FeatureX87, FeatureCMPXCHG8B, FeatureMMX,
                               [TuningSlowUAMem16, TuningInsertVZEROUPPER]>;
 
 // We also provide a generic 64-bit specific x86 processor model which tries to
 // be good for modern chips without enabling instruction set encodings past the
 // basic SSE2 and 64-bit ones. It disables slow things from any mainstream and
 // modern 64-bit x86 chip, and enables features that are generally beneficial.
 //
 // We currently use the Sandy Bridge model as the default scheduling model as
 // we use it across Nehalem, Westmere, Sandy Bridge, and Ivy Bridge which
 // covers a huge swath of x86 processors. If there are specific scheduling
 // knobs which need to be tuned differently for AMD chips, we might consider
 // forming a common base for them.
 def : ProcModel<"x86-64", SandyBridgeModel, ProcessorFeatures.X86_64V1Features,
 [
   TuningSlow3OpsLEA,
   TuningSlowDivide64,
   TuningSlowIncDec,
   TuningMacroFusion,
   TuningInsertVZEROUPPER
 ]>;
 
 // x86-64 micro-architecture levels.
 def : ProcModel<"x86-64-v2", SandyBridgeModel, ProcessorFeatures.X86_64V2Features,
                 ProcessorFeatures.SNBTuning>;
 // Close to Haswell.
 def : ProcModel<"x86-64-v3", HaswellModel, ProcessorFeatures.X86_64V3Features,
                 ProcessorFeatures.HSWTuning>;

llvm/test/CodeGen/X86/segmented-stacks-dynamic.ll

 ; X86-NEXT:    subl $8, %esp
 ; X86-NEXT:    pushl %esi
 ; X86-NEXT:    pushl %eax
 ; X86-NEXT:    calll dummy_use@PLT
 ; X86-NEXT:    addl $16, %esp
 ; X86-NEXT:    testl %esi, %esi
 ; X86-NEXT:    je .LBB0_6
 ; X86-NEXT:  # %bb.8: # %false
-; X86-NEXT:    addl $-1, %esi
+; X86-NEXT:    decl %esi
 ; X86-NEXT:    subl $12, %esp
 ; X86-NEXT:    pushl %esi
 ; X86-NEXT:    calll test_basic@PLT
 ; X86-NEXT:    jmp .LBB0_7
 ; X86-NEXT:  .LBB0_6: # %true
 ; X86-NEXT:    xorl %eax, %eax
 ; X86-NEXT:  .LBB0_7: # %true
 ; X86-NEXT:    leal -4(%ebp), %esp

 ; X64-NEXT:    callq __morestack_allocate_stack_space
 ; X64-NEXT:    movq %rax, %rdi
 ; X64-NEXT:  .LBB0_5:
 ; X64-NEXT:    movl %ebx, %esi
 ; X64-NEXT:    callq dummy_use@PLT
 ; X64-NEXT:    testl %ebx, %ebx
 ; X64-NEXT:    je .LBB0_6
 ; X64-NEXT:  # %bb.8: # %false
-; X64-NEXT:    addl $-1, %ebx
+; X64-NEXT:    decl %ebx
 ; X64-NEXT:    movl %ebx, %edi
 ; X64-NEXT:    callq test_basic@PLT
 ; X64-NEXT:    jmp .LBB0_7
 ; X64-NEXT:  .LBB0_6: # %true
 ; X64-NEXT:    xorl %eax, %eax
 ; X64-NEXT:  .LBB0_7: # %true
 ; X64-NEXT:    leaq -8(%rbp), %rsp
 ; X64-NEXT:    popq %rbx

 ; X32ABI-NEXT:    callq __morestack_allocate_stack_space
 ; X32ABI-NEXT:    movl %eax, %edi
 ; X32ABI-NEXT:  .LBB0_5:
 ; X32ABI-NEXT:    movl %ebx, %esi
 ; X32ABI-NEXT:    callq dummy_use@PLT
 ; X32ABI-NEXT:    testl %ebx, %ebx
 ; X32ABI-NEXT:    je .LBB0_6
 ; X32ABI-NEXT:  # %bb.8: # %false
-; X32ABI-NEXT:    addl $-1, %ebx
+; X32ABI-NEXT:    decl %ebx
 ; X32ABI-NEXT:    movl %ebx, %edi
 ; X32ABI-NEXT:    callq test_basic@PLT
 ; X32ABI-NEXT:    jmp .LBB0_7
 ; X32ABI-NEXT:  .LBB0_6: # %true
 ; X32ABI-NEXT:    xorl %eax, %eax
 ; X32ABI-NEXT:  .LBB0_7: # %true
 ; X32ABI-NEXT:    leal -8(%ebp), %esp
 ; X32ABI-NEXT:    popq %rbx

llvm/test/CodeGen/X86/twoaddr-lea.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mcpu=generic -mtriple=x86_64-apple-darwin | FileCheck %s
 
 ;; X's live range extends beyond the shift, so the register allocator
 ;; cannot coalesce it with Y.  Because of this, a copy needs to be
 ;; emitted before the shift to save the register value before it is
 ;; clobbered.  However, this copy is not needed if the register
 ;; allocator turns the shift into an LEA.  This also occurs for ADD.
 
 ; Check that the shift gets turned into an LEA.

 ; CHECK-NEXT:    .p2align 4, 0x90
 ; CHECK-NEXT:  LBB3_4: ## %bb4
 ; CHECK-NEXT:    ## Parent Loop BB3_3 Depth=1
 ; CHECK-NEXT:    ## => This Inner Loop Header: Depth=2
 ; CHECK-NEXT:    testl %esi, %esi
 ; CHECK-NEXT:    jne LBB3_9
 ; CHECK-NEXT:  ## %bb.5: ## %bb5
 ; CHECK-NEXT:    ## in Loop: Header=BB3_4 Depth=2
-; CHECK-NEXT:    addq $1, %rdx
+; CHECK-NEXT:    incq %rdx
 ; CHECK-NEXT:    cmpq %rcx, %rdx
 ; CHECK-NEXT:    jl LBB3_4
 ; CHECK-NEXT:    jmp LBB3_3
 ; CHECK-NEXT:  LBB3_9: ## %bb8
 ; CHECK-NEXT:    ud2
 bb:
   br label %bb1
 

llvm/test/MC/X86/x86-directive-nops-errors.s

 # RUN: not llvm-mc -triple i386 %s -filetype=obj -o /dev/null 2>&1 | FileCheck --check-prefix=X86 %s
-# RUN: not llvm-mc -triple=x86_64 %s -filetype=obj -o /dev/null 2>&1 | FileCheck --check-prefix=X64 %s
+# RUN: not llvm-mc -triple=x86_64 -mcpu=x86-64 %s -filetype=obj -o /dev/null 2>&1 | FileCheck --check-prefix=X64 %s
 
 .nops 4, 3
 # X86: :[[@LINE-1]]:1: error: illegal NOP size 3.
 .nops 4, 4
 # X86: :[[@LINE-1]]:1: error: illegal NOP size 4.
 .nops 4, 5
 # X86: :[[@LINE-1]]:1: error: illegal NOP size 5.
 .nops 16, 15
 # X86: :[[@LINE-1]]:1: error: illegal NOP size 15.
 # X64: :[[@LINE-2]]:1: error: illegal NOP size 15.