Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug in _use_lto_suffix logic #74

Closed
xuanruiqi opened this issue Feb 7, 2023 · 10 comments
Closed

Bug in _use_lto_suffix logic #74

xuanruiqi opened this issue Feb 7, 2023 · 10 comments

Comments

@xuanruiqi
Copy link

There is this logic in the PKGBUILD:

if [[ "$_use_llvm_lto" = "thin" || "$_use_llvm_lto" = "full" && -n "$_use_lto_suffix" ]]; then

This is apparently not correct. There should be parens around the or expression, otherwise the expression will always evaluate as true as long as $_use_llvm_lto is "thin".
@sirlucjan
Copy link
Member

sirlucjan commented Feb 7, 2023 •
edited
Loading

I set such a value:

# Clang LTO mode, only available with the "llvm" compiler - options are "none", "full" or "thin".
# ATTENTION - one of three predefined values should be selected!
# "full: uses 1 thread for Linking, slow and uses more memory, theoretically with the highest performance gains."
# "thin: uses multiple threads, faster and uses less memory, may have a lower runtime performance than Full."
# "none: disable LTO
_use_llvm_lto=${_use_llvm_lto-full}

It correctly added the suffix -lto

==> Making package: linux-bore-lto 6.1.10-2 (wto, 7 lut 2023, 14:28:28)
==> WARNING: Skipping dependency checks.
==> Retrieving sources...
  -> Found linux-6.1.10.tar.xz
  -> Found config
  -> Found auto-cpu-optimization.sh
  -> Found 0001-cachyos-base-all.patch
  -> Found 0001-Add-latency-priority-for-CFS-class.patch
  -> Found 0001-bore-cachy.patch
==> Validating source files with sha256sums...
    linux-6.1.10.tar.xz ... Passed
    config ... Passed
    auto-cpu-optimization.sh ... Passed
    0001-cachyos-base-all.patch ... Passed
    0001-Add-latency-priority-for-CFS-class.patch ... Passed
    0001-bore-cachy.patch ... Passed

It also set the config correctly:

----------------------------------
| APPLYING AUTO-CPU-OPTIMIZATION |
----------------------------------
[*] DETECTED CPU (MARCH) : MSKYLAKE
Enabling CachyOS config...
Selecting BORE CPU scheduler...
Selecting 'full' LLVM level...
Setting tick rate to 500Hz...
Setting default NR_CPUS...
Disabling MQ-Deadline I/O scheduler...
Disabling Kyber I/O scheduler...
Setting performance governor...
Selecting 'full' tick type...
Selecting 'full' preempt type...
Enabling KBUILD_CFLAGS -O3...
Disabling TCP_CONG_CUBIC...
Selecting 'standard' LRU_GEN config...
Selecting 'none' PER_VMA_LOCK config...
Selecting 'always' TRANSPARENT_HUGEPAGE config...
Selecting 'normal' ZSTD modules and kernel compression level...

With another value, like:

# Clang LTO mode, only available with the "llvm" compiler - options are "none", "full" or "thin".
# ATTENTION - one of three predefined values should be selected!
# "full: uses 1 thread for Linking, slow and uses more memory, theoretically with the highest performance gains."
# "thin: uses multiple threads, faster and uses less memory, may have a lower runtime performance than Full."
# "none: disable LTO
_use_llvm_lto=${_use_llvm_lto-papa}

Suffix lto is not added.

==> Making package: linux-bore 6.1.10-2 (wto, 7 lut 2023, 14:21:58)
==> WARNING: Skipping dependency checks.
==> Retrieving sources...
  -> Found linux-6.1.10.tar.xz
  -> Found config
  -> Found auto-cpu-optimization.sh
  -> Found 0001-cachyos-base-all.patch
  -> Found 0001-Add-latency-priority-for-CFS-class.patch
  -> Found 0001-bore-cachy.patch
==> Validating source files with sha256sums...
    linux-6.1.10.tar.xz ... Passed
    config ... Passed
    auto-cpu-optimization.sh ... Passed
    0001-cachyos-base-all.patch ... Passed
    0001-Add-latency-priority-for-CFS-class.patch ... Passed
    0001-bore-cachy.patch ... Passed

The compilation also fails:

----------------------------------
| APPLYING AUTO-CPU-OPTIMIZATION |
----------------------------------
[*] DETECTED CPU (MARCH) : MSKYLAKE
Enabling CachyOS config...
Selecting BORE CPU scheduler...
==> ERROR: The value 'papa' is invalid. Choose the correct one again.

@xuanruiqi
Copy link
Author

Can you try an empty $_use_lto_suffix with Thin LTO? This for me causes the no suffix to be ignored, and I've pointed out what I think is the problem. It turns out that in Bash, short-circuit boolean operators are always executed from left to right with the same precedence, hence the unexpected behavior.

@sirlucjan
Copy link
Member

Here's the problem. Yes, you're right - I'll get right on it.

sirlucjan added a commit that referenced this issue Feb 7, 2023
@sirlucjan
Fix -lto suffix (#74)
55b5004 
Signed-off-by: Piotr Gorski <[email protected]>
@sirlucjan
Copy link
Member

@xuanruiqi

Could you try now?

@sirlucjan
Copy link
Member 

# Clang LTO mode, only available with the "llvm" compiler - options are "none", "full" or "thin".
# ATTENTION - one of three predefined values should be selected!
# "full: uses 1 thread for Linking, slow and uses more memory, theoretically with the highest performance gains."
# "thin: uses multiple threads, faster and uses less memory, may have a lower runtime performance than Full."
# "none: disable LTO
_use_llvm_lto=${_use_llvm_lto-full}

# Use suffix -lto only when requested by the user
# Enabled by default.
# If you do not want the suffix -lto remove the "y" sign next to the flag.
# https:/CachyOS/linux-cachyos/issues/36
_use_lto_suffix=${_use_lto_suffix-}

And results:

==> Making package: linux-bore 6.1.10-2 (wto, 7 lut 2023, 14:45:38)
==> WARNING: Skipping dependency checks.
==> Retrieving sources...
  -> Found linux-6.1.10.tar.xz
  -> Found config
  -> Found auto-cpu-optimization.sh
  -> Found 0001-cachyos-base-all.patch
  -> Found 0001-Add-latency-priority-for-CFS-class.patch
  -> Found 0001-bore-cachy.patch
==> Validating source files with sha256sums...
    linux-6.1.10.tar.xz ... Passed
    config ... Passed
    auto-cpu-optimization.sh ... Passed
    0001-cachyos-base-all.patch ... Passed
    0001-Add-latency-priority-for-CFS-class.patch ... Passed
    0001-bore-cachy.patch ... Passed

Full lto is setting properly:

----------------------------------
| APPLYING AUTO-CPU-OPTIMIZATION |
----------------------------------
[*] DETECTED CPU (MARCH) : MSKYLAKE
Enabling CachyOS config...
Selecting BORE CPU scheduler...
Selecting 'full' LLVM level...
Setting tick rate to 500Hz...
Setting default NR_CPUS...
Disabling MQ-Deadline I/O scheduler...
Disabling Kyber I/O scheduler...
Setting performance governor...
Selecting 'full' tick type...
Selecting 'full' preempt type...
Enabling KBUILD_CFLAGS -O3...
Disabling TCP_CONG_CUBIC...
Selecting 'standard' LRU_GEN config...
Selecting 'none' PER_VMA_LOCK config...
Selecting 'always' TRANSPARENT_HUGEPAGE config...
Selecting 'normal' ZSTD modules and kernel compression level...

@xuanruiqi
Copy link
Author

Works for me!

@sirlucjan
Copy link
Member

sirlucjan commented Feb 7, 2023 •
edited
Loading

In this case, if you find the solution working, the thread can be closed. Now admittedly, when selecting another value that we did not anticipate will add suffix -lto (but with the earlier solution it was exactly the same):

# Clang LTO mode, only available with the "llvm" compiler - options are "none", "full" or "thin".
# ATTENTION - one of three predefined values should be selected!
# "full: uses 1 thread for Linking, slow and uses more memory, theoretically with the highest performance gains."
# "thin: uses multiple threads, faster and uses less memory, may have a lower runtime performance than Full."
# "none: disable LTO
_use_llvm_lto=${_use_llvm_lto-papa}

# Use suffix -lto only when requested by the user
# Enabled by default.
# If you do not want the suffix -lto remove the "y" sign next to the flag.
# https:/CachyOS/linux-cachyos/issues/36
_use_lto_suffix=${_use_lto_suffix-y}

==> Making package: linux-bore-lto 6.1.10-2 (wto, 7 lut 2023, 14:50:30)
==> WARNING: Skipping dependency checks.
==> Retrieving sources...
  -> Found linux-6.1.10.tar.xz
  -> Found config
  -> Found auto-cpu-optimization.sh
  -> Found 0001-cachyos-base-all.patch
  -> Found 0001-Add-latency-priority-for-CFS-class.patch
  -> Found 0001-bore-cachy.patch
==> Validating source files with sha256sums...
    linux-6.1.10.tar.xz ... Passed
    config ... Passed
    auto-cpu-optimization.sh ... Passed
    0001-cachyos-base-all.patch ... Passed
    0001-Add-latency-priority-for-CFS-class.patch ... Passed
    0001-bore-cachy.patch ... Passed

However, the compilation will not succeed anyway:

----------------------------------
| APPLYING AUTO-CPU-OPTIMIZATION |
----------------------------------
[*] DETECTED CPU (MARCH) : MSKYLAKE
Enabling CachyOS config...
Selecting BORE CPU scheduler...
==> ERROR: The value 'papa' is invalid. Choose the correct one again.

@xuanruiqi So I guess it can be considered that everything works as it should?

sirlucjan added a commit that referenced this issue Feb 7, 2023
@sirlucjan
Fix -lto suffix (#74)
d0fa84a 
Signed-off-by: Piotr Gorski <[email protected]>
@sirlucjan
Copy link
Member

@xuanruiqi I tried to do it another way.

# Clang LTO mode, only available with the "llvm" compiler - options are "none", "full" or "thin".
# ATTENTION - one of three predefined values should be selected!
# "full: uses 1 thread for Linking, slow and uses more memory, theoretically with the highest performance gains."
# "thin: uses multiple threads, faster and uses less memory, may have a lower runtime performance than Full."
# "none: disable LTO
_use_llvm_lto=${_use_llvm_lto-thin}

# Use suffix -lto only when requested by the user
# Enabled by default.
# If you do not want the suffix -lto remove the "y" sign next to the flag.
# https:/CachyOS/linux-cachyos/issues/36
_use_lto_suffix=${_use_lto_suffix-}

and here we go:

==> Making package: linux-bore 6.1.10-2 (wto, 7 lut 2023, 16:00:59)
==> WARNING: Skipping dependency checks.
==> Retrieving sources...
  -> Found linux-6.1.10.tar.xz
  -> Found config
  -> Found auto-cpu-optimization.sh
  -> Found 0001-cachyos-base-all.patch
  -> Found 0001-Add-latency-priority-for-CFS-class.patch
  -> Found 0001-bore-cachy.patch
==> Validating source files with sha256sums...
    linux-6.1.10.tar.xz ... Passed
    config ... Passed
    auto-cpu-optimization.sh ... Passed
    0001-cachyos-base-all.patch ... Passed
    0001-Add-latency-priority-for-CFS-class.patch ... Passed
    0001-bore-cachy.patch ... Passed

and

----------------------------------
| APPLYING AUTO-CPU-OPTIMIZATION |
----------------------------------
[*] DETECTED CPU (MARCH) : MSKYLAKE
Enabling CachyOS config...
Selecting BORE CPU scheduler...
Selecting 'thin' LLVM level...
Setting tick rate to 500Hz...
Setting default NR_CPUS...
Disabling MQ-Deadline I/O scheduler...
Disabling Kyber I/O scheduler...
Setting performance governor...
Selecting 'full' tick type...
Selecting 'full' preempt type...
Enabling KBUILD_CFLAGS -O3...
Disabling TCP_CONG_CUBIC...
Selecting 'standard' LRU_GEN config...
Selecting 'none' PER_VMA_LOCK config...
Selecting 'always' TRANSPARENT_HUGEPAGE config...
Selecting 'normal' ZSTD modules and kernel compression level...

Could you confirm?

@sirlucjan sirlucjan reopened this Feb 7, 2023
@xuanruiqi
Copy link
Author

Looks good & works for me

@sirlucjan
Copy link
Member

Great. Thanks again for your report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sirlucjan @xuanruiqi