- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 23.5k
 
Use more explicit architecture compiler options on Linux #64366
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.
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
base: master
Are you sure you want to change the base?
Conversation
4b944e8    to
    1fbc46c      
    Compare
  
    1fbc46c    to
    75ab985      
    Compare
  
    75ab985    to
    76cb809      
    Compare
  
    76cb809    to
    f1d8e95      
    Compare
  
    f1d8e95    to
    a2fcfe0      
    Compare
  
    a2fcfe0    to
    ef0459d      
    Compare
  
    ef0459d    to
    df70a63      
    Compare
  
    df70a63    to
    c3ba779      
    Compare
  
    c3ba779    to
    be137d1      
    Compare
  
    be137d1    to
    4767aee      
    Compare
  
    4767aee    to
    2e3fbc8      
    Compare
  
    2e3fbc8    to
    722cf1f      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to have missed some cross-compilation code further down that does some of this already, which should likely be removed. The hardcoded lib folder path are actually bogus on many distros, but shouldn't be needed(?).
diff --git a/platform/linuxbsd/detect.py b/platform/linuxbsd/detect.py
index 72bffceb1f..02d8d20fbf 100644
--- a/platform/linuxbsd/detect.py
+++ b/platform/linuxbsd/detect.py
@@ -469,16 +469,6 @@ def configure(env: "Environment"):
     if platform.system() == "FreeBSD":
         env.Append(LINKFLAGS=["-lkvm"])
 
-    ## Cross-compilation
-    # TODO: Support cross-compilation on architectures other than x86.
-    host_is_64_bit = sys.maxsize > 2**32
-    if host_is_64_bit and env["arch"] == "x86_32":
-        env.Append(CCFLAGS=["-m32"])
-        env.Append(LINKFLAGS=["-m32", "-L/usr/lib/i386-linux-gnu"])
-    elif not host_is_64_bit and env["arch"] == "x86_64":
-        env.Append(CCFLAGS=["-m64"])
-        env.Append(LINKFLAGS=["-m64", "-L/usr/lib/i686-linux-gnu"])
-
     # Link those statically for portability
     if env["use_static_cpp"]:
         env.Append(LINKFLAGS=["-static-libgcc", "-static-libstdc++"])Edit: I'm moving this up in #84307 which will conflict with this PR, but it's a safe-ish fixup for 4.2. You can rebase and remove that whole block as yours supersedes it fully.
        
          
                platform/linuxbsd/detect.py
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are both -m64/-m32 and -march needed at the same time, or does the latter make the old -m64 and -m32 flags redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are both required, yes.
If you try to use -march= on x86 without the proper bitness or specifying -m32/-m64 it will not work.
While confusing, -m32/-m64 specifically refers to x86_32 and x86_64, not 32-bit and 64-bit in general.
722cf1f    to
    87bb942      
    Compare
  
    988ce27    to
    010b6bb      
    Compare
  
    2eb9dfc    to
    408acaa      
    Compare
  
    408acaa    to
    45138b7      
    Compare
  
    45138b7    to
    9365198      
    Compare
  
    9365198    to
    f2123a5      
    Compare
  
    f2123a5    to
    893edff      
    Compare
  
    893edff    to
    e3b592c      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, it works as expected.
elfx86exts output before and after on a Linux release export template:
File format and CPU architecture: Elf, X86_64
MODE64 (call)
SSE2 (movdqu)
SSE1 (movups)
CMOV (cmovne)
MMX (movq)
BMI (tzcnt)
AES (aesenc)
PCLMUL (pclmulqdq)
NOT64BITMODE (xchg)
BMI2 (shlx)
Instruction set extensions used: AES, BMI, BMI2, CMOV, MMX, MODE64, NOT64BITMODE, PCLMUL, SSE1, SSE2
CPU Generation: Unknown
Binaries before and after this PR are byte-for-byte identical in size too, so there's no change in the CPU optimizations applied.
e3b592c    to
    0ce8030      
    Compare
  
    0ce8030    to
    83ac919      
    Compare
  
    83ac919    to
    a0a1889      
    Compare
  
    a0a1889    to
    d2049ac      
    Compare
  
    | env.Prepend(CFLAGS=["-m64", "-march=x86-64"]) | ||
| env.Prepend(CXXFLAGS=["-m64", "-march=x86-64"]) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use CCFLAGS for those if it's the same for C and C++.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see #64366 (comment). This should be documented with a comment.
But this is also exactly why I haven't fully been able to approve this PR, as I'm worried about exactly these implications of forcing some defaults which may not be what users want to use. We already have similar issues with use_llvm=yes forcing CC=clang CXX=clang++ which can also be problematic, so it's not new to be clear.
But there are implications to explicitly spelling out default values like this. That being said I also see value in being explicit about what we expect to be used by default.
I started some WIP in the same area but didn't have time to continue. The scope is bigger than just Linux, and it includes an option to skip setting these defaults:
akien-mga@1e4aef2 (branch https://github.com/akien-mga/godot/tree/scons-explicit-march-mtune)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your branch, why is it needed to have a env["default_arch_flags"] bool to disable the default arch flags? If users desire specific arch flags, they should be able to override it as @MonterraByte mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. In this case, it's fine to always set -march, because if the user sets -march a second time, it will override the first value. This is true for both GCC and Clang. (Not for MSVC, but that's not relevant here.)
You can verify this with gcc -march=x86-64 -march=x86-64-v3 -dM -E - < /dev/null | grep AVX.
d2049ac    to
    09aafda      
    Compare
  
    09aafda    to
    98e9fc9      
    Compare
  
    Co-authored-by: Joaquim Monteiro <[email protected]>
98e9fc9    to
    1e4cf9f      
    Compare
  
    
This PR takes a piece out of #55778. Specifically, this PR was driven by this review comment from @akien-mga:
I tested this PR on x86_64, arm64, and rv64 hosts & targets, and compiling x86_32 on x86_64.