[Driver] Correct comment on default for -falign-functions

[pogo59]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Paul T Robinson (pogo59)

Changes

-falign-functions with no argument is supposed to be equivalent to -falign-functions=16, according to the documentation. Make it so.

---

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

2 Files Affected:

• (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+1-1)
• (modified) clang/test/Driver/function-alignment.c (+2-1)

diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 1e37d9d348818..bc74f9afcf868 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1982,7 +1982,7 @@ unsigned tools::ParseFunctionAlignment(const ToolChain &TC,
     return 0;
 
   if (A->getOption().matches(options::OPT_falign_functions))
-    return 0;
+    return 4; // log2(16)
 
   unsigned Value = 0;
   if (StringRef(A->getValue()).getAsInteger(10, Value) || Value > 65536)
diff --git a/clang/test/Driver/function-alignment.c b/clang/test/Driver/function-alignment.c
index ed4cc80f7ed4f..b0eb7cfdad8cc 100644
--- a/clang/test/Driver/function-alignment.c
+++ b/clang/test/Driver/function-alignment.c
@@ -1,5 +1,5 @@
 // RUN: %clang -### %s 2>&1 | FileCheck %s -check-prefix CHECK-0
-// RUN: %clang -### -falign-functions %s 2>&1 | FileCheck %s -check-prefix CHECK-1
+// RUN: %clang -### -falign-functions %s 2>&1 | FileCheck %s -check-prefix CHECK-16
 // RUN: %clang -### -falign-functions=1 %s 2>&1 | FileCheck %s -check-prefix CHECK-1
 // RUN: %clang -### -falign-functions=2 %s 2>&1 | FileCheck %s -check-prefix CHECK-2
 // RUN: %clang -### -falign-functions=3 %s 2>&1 | FileCheck %s -check-prefix CHECK-3
@@ -8,6 +8,7 @@
 // RUN: not %clang -### -falign-functions=a %s 2>&1 | FileCheck %s -check-prefix CHECK-ERR-A
 
 // CHECK-0-NOT: "-function-alignment"
+// CHECK-16: "-function-alignment" "4"
 // CHECK-1-NOT: "-function-alignment"
 // CHECK-2: "-function-alignment" "1"
 // CHECK-3: "-function-alignment" "2"

[compnerd]

This is not true. Quoting the documentation:

If n is not specified or is zero, use a machine-dependent default. The maximum allowed n option value is 65536.

16 is a reasonable default for X86, but I don't think that it universally makes sense. Can we switch based on the architecture and continue to return a default of 0?

[pogo59]

Ah, I should have said "the commentary" on ParseFunctionAlignment, which does say that no argument means 16.
Which documentation are you quoting? I don't see this option described in the clang user manual.

I'm not aware that the driver, or clang generally, knows about preferred alignment by target, but I guess we can invent a parameter for that.

[MaskRay]

I agree that clangDriver should not hard code a value like 16.

The relevant code is at llvm/lib/CodeGen/MachineFunction.cpp

  // FIXME: Shouldn't use pref alignment if explicit alignment is set on F.
  // FIXME: Use Function::hasOptSize().
  if (!F.hasFnAttribute(Attribute::OptimizeForSize))
    Alignment = std::max(Alignment,
                         STI->getTargetLowering()->getPrefFunctionAlignment());

[compnerd]

@pogo59 that is documented in the GCC documentation. Many, if not most, of the driver arguments are GCC derived and behave identically.

[pogo59]

The driver can't really be poking down into LLVM's TargetLowering; at best it could be querying Clang's TargetInfo, which knows a lot about data alignment but does not currently have any knowledge of code alignment. So, I've reverted the change and instead fixed the incorrect comment.

[MaskRay]

LGTM.

The patch title/description need to be changed to ensure "Squash and merge" gets the right message (the git commit message is ignored).