@llvm/pr-subscribers-backend-x86

Author: Peter Collingbourne (pcc)

diff --git a/llvm/lib/CodeGen/MachineFunction.cpp b/llvm/lib/CodeGen/MachineFunction.cpp
index 38ad582ba923c..429a17a9113d3 100644
--- a/llvm/lib/CodeGen/MachineFunction.cpp
+++ b/llvm/lib/CodeGen/MachineFunction.cpp
@@ -211,9 +211,8 @@ void MachineFunction::init() {
   ConstantPool = new (Allocator) MachineConstantPool(getDataLayout());
   Alignment = STI->getTargetLowering()->getMinFunctionAlignment();
 
-  // FIXME: Shouldn't use pref alignment if explicit alignment is set on F.
   // FIXME: Use Function::hasOptSize().
-  if (!F.hasFnAttribute(Attribute::OptimizeForSize))
+  if (!F.getAlign() && !F.hasFnAttribute(Attribute::OptimizeForSize))
     Alignment = std::max(Alignment,
                          STI->getTargetLowering()->getPrefFunctionAlignment());
 
diff --git a/llvm/test/CodeGen/X86/function-align.ll b/llvm/test/CodeGen/X86/function-align.ll
new file mode 100644
index 0000000000000..11d0e99929927
--- /dev/null
+++ b/llvm/test/CodeGen/X86/function-align.ll
@@ -0,0 +1,18 @@
+; RUN: llc -function-sections < %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: .section .text.f1
+; CHECK-NOT: .p2align
+; CHECK: f1:
+define void @f1() align 1 {
+  ret void
+}
+
+; CHECK: .section .text.f2
+; CHECK-NEXT: .globl f2
+; CHECK-NEXT: .p2align 1
+define void @f2() align 2 {
+  ret void
+}

This has a side-effect you might not want.

Due to the way member function pointers work in the Itanium ABI, all member functions must have at least 2 byte alignment. clang represents this by marking each member function "align 2". This used to allow increasing the alignment if it wasn't specified, but with this patch, all member functions have exactly 2-byte alignment.

Interesting. I had not considered that consequence.

I think longer term we will want a way for frontends to query the subtarget's minimum and preferred alignment. Then here clang could query the right one based on whether it's optimizing for size and set the align attribute to the max of that and 2.

Maybe for now we can introduce a min-align attribute and have clang set it to 2 on member functions.

The alignment is queried directly by optimizations through getPointerAlignment; I don't think we can decrease it after the fact.

I'd suggest adding an attribute in the opposite direction; for functions where we don't want the alignment to increase, add a function attribute "minimize-alignment", and the backend won't increase alignment for those functions.

Oh, also, on the subject of function alignment, we currently still have

The alignment is queried directly by optimizations through getPointerAlignment; I don't think we can decrease it after the fact.

The idea is that min-align would only be capable of increasing the function alignment, not decreasing it. For example, given a function with min-align 2, on x86 with -Os we would end up with alignment 2 and x86 without -Os would have alignment 16, and on aarch64 the alignment would still always be at least 4.

The idea is that min-align would only be capable of increasing the function alignment, not decreasing it.

Then getPointerAlignment() would be overly conservative, which would prevent us from removing masking operations. Unless you make getPointerAlignment() also check for the attribute, which... technically works, I guess, but is pretty confusing.

Longer-term, I'd prefer to switch Function::getAlign() to return an Align instead of a MaybeAlign.

Then getPointerAlignment() would be overly conservative, which would prevent us from removing masking operations. Unless you make getPointerAlignment() also check for the attribute, which... technically works, I guess, but is pretty confusing.

Another consideration is that we may want the object file to have a separate minimum alignment and preferred alignment for a section. This supports the CFI jump table use case that I'm working on, see #149448 and #150151, but I imagine it could also be useful for other section placement techniques. If we did go with the different alignments in the object file I would expect this to be represented using two attributes at the IR level which would be respected with getPointerAlignment().

Longer-term, I'd prefer to switch Function::getAlign() to return an Align instead of a MaybeAlign.

That seems like a good direction to me.

clang represents this by marking each member function "align 2". This used to allow increasing the alignment if it wasn't specified, but with this patch, all member functions have exactly 2-byte alignment.

Interesting!

For reference, this happens here:

We hit an interesting bug triggered by this, due to Windows ASan's interception mechanism implicitly relying on functions having higher alignment. (https://crbug.com/437182411)

As discussed above, I don't think the intention of the Clang code was ever to lower the alignment. Do you have any ideas how to best preserve 16-byte alignment at least on Windows x64 for now?

We've never promised any function alignment on x86, beyond the 2-byte alignment restriction for member functions. At -O2, we normally prefer 16-byte alignment, but that's just a heuristic, and we don't align at -Os/-Oz. If asan requires higher alignment for some reason, we should model that in the compiler somewhere (maybe in the asan lowering pass).

But it might make sense to revert this patch until we've worked out the overall approach here.

If asan requires higher alignment for some reason, we should model that in the compiler somewhere (maybe in the asan lowering pass).

In our bug, it's actually not the asan-instrumented code that required 16-byte alignment, but the asan runtime itself. So regardless of this change, we should probably pass -falign-functions` to request that explicitly: #154694

I'd like this PR to be reverted until https://discourse.llvm.org/t/rfc-enhancing-function-alignment-attributes/88019 concludes.