Page MenuHomePhabricator

[CGCall] Annotate pointer argument with alignment
Changes PlannedPublic

Authored by lebedev.ri on Apr 2 2021, 2:05 AM.

Details

Summary

Same idea as D99790.

As it is being noted in D99249, lack of alignment information on this has been preventing LICM from happening.
For some time now, lack of alignment attribute does *not* imply natural alignment, but an alignment of 1.

This causes quite a bit of clang tests to tail:

Testing Time: 167.48s
  Unsupported      :    69
  Passed           : 27038
  Expectedly Failed:    29
  Failed           :   370
FAILED: tools/clang/test/CMakeFiles/check-clang

so before i pointlessly go mad updating each one of them,
is this generally correct, or are we not allowed to do this?
Are there some further preconditions missing?

Diff Detail

Unit TestsFailed

TimeTest
90 msx64 debian > Clang.CodeGen::catch-alignment-assumption-attribute-align_value-on-lvalue.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -emit-llvm /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-align_value-on-lvalue.cpp -o - -triple x86_64-linux-gnu | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-align_value-on-lvalue.cpp
80 msx64 debian > Clang.CodeGen::catch-alignment-assumption-attribute-align_value-on-paramvar.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -emit-llvm /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-align_value-on-paramvar.cpp -o - -triple x86_64-linux-gnu | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-align_value-on-paramvar.cpp --check-prefixes=CHECK,CHECK-NOSANITIZE
90 msx64 debian > Clang.CodeGen::catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -emit-llvm /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp -o - -triple x86_64-linux-gnu | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp
60 msx64 debian > Clang.CodeGen::catch-alignment-assumption-attribute-alloc_align-on-function.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -emit-llvm /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function.cpp -o - -triple x86_64-linux-gnu | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function.cpp --check-prefixes=CHECK,CHECK-NOSANITIZE
70 msx64 debian > Clang.CodeGen::catch-alignment-assumption-attribute-assume_aligned-on-function-two-params.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -emit-llvm /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-assume_aligned-on-function-two-params.cpp -o - -triple x86_64-linux-gnu | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/catch-alignment-assumption-attribute-assume_aligned-on-function-two-params.cpp
View Full Test Results (39 Failed)

Event Timeline

lebedev.ri requested review of this revision.Apr 2 2021, 2:05 AM
lebedev.ri created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2021, 2:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This feels scary: the C standard technically allows this, but we haven't done it in the past, and it could break otherwise functioning code. (We've only assumed alignment about pointers that are dereferenced/dereferenceable.)

For D99790, we're already marking the pointer dereferenceable, so also marking the alignment seems like a small extra step. (Really, it's a regression fix; we used to treat dereferenceable as implying alignment. I guess I missed a spot when I was fixing that.) But here, we're not assuming it's dereferenceable at the moment. So there's more potential to break code, and also the potential benefit is small.

@efriedma thank you for taking a look!
I agree that D99790 is basically guaranteed safe, and this "probably" isn't.

This feels scary: the C standard technically allows this, but we haven't done it in the past, and it could break otherwise functioning code. (We've only assumed alignment about pointers that are dereferenced/dereferenceable.)

I can add necessary UBSan plumbing beforehand, iff we can actually do this.

For D99790, we're already marking the pointer dereferenceable, so also marking the alignment seems like a small extra step. (Really, it's a regression fix; we used to treat dereferenceable as implying alignment. I guess I missed a spot when I was fixing that.)

Yep. Feel like stamping that one? :)

But here, we're not assuming it's dereferenceable at the moment. So there's more potential to break code, and also the potential benefit is small.

The last major conversation we had about this was this RFC I sent out about five years ago:

https://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html

In that RFC, I specifically argued that we should not do this, and that it should only be considered undefined behavior to actually access memory through a misaligned pointer (for a particular definition of "access memory"). At Apple, we have made that a promise to our internal users, so even if we decide to do this, we will need to not do it on Darwin. However, as I remember it, the LLVM community did not reach a consensus to adopt my recommendation, so in principle we still have the flexibility to start doing this. I continue to believe that doing so would be a mistake. At any rate, you should start by reading that thread.

lebedev.ri planned changes to this revision.Apr 6 2021, 2:26 PM

@rjmccall thank you for taking a look!

The last major conversation we had about this was this RFC I sent out about five years ago:

https://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html

In that RFC, I specifically argued that we should not do this, and that it should only be considered undefined behavior to actually access memory through a misaligned pointer (for a particular definition of "access memory"). At Apple, we have made that a promise to our internal users, so even if we decide to do this, we will need to not do it on Darwin. However, as I remember it, the LLVM community did not reach a consensus to adopt my recommendation, so in principle we still have the flexibility to start doing this. I continue to believe that doing so would be a mistake. At any rate, you should start by reading that thread.

Thank you for the pointer!
I can for sure provide an opt-out, however note that in the end it will cause performance regressions as compared to the current LLVM optimizations.
I will start with an UBSan part then.

@rjmccall thank you for taking a look!

The last major conversation we had about this was this RFC I sent out about five years ago:

https://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html

In that RFC, I specifically argued that we should not do this, and that it should only be considered undefined behavior to actually access memory through a misaligned pointer (for a particular definition of "access memory"). At Apple, we have made that a promise to our internal users, so even if we decide to do this, we will need to not do it on Darwin. However, as I remember it, the LLVM community did not reach a consensus to adopt my recommendation, so in principle we still have the flexibility to start doing this. I continue to believe that doing so would be a mistake. At any rate, you should start by reading that thread.

Thank you for the pointer!
I can for sure provide an opt-out, however note that in the end it will cause performance regressions as compared to the current LLVM optimizations.

How so? We do not currently assume that pointer arguments are aligned, or even dereferenceable.

I don't mind making stronger assumptions about the C++ this argument, but we really should not do this for arbitrary pointer arguments. If you want to put together a UBSan mode that checks this, that's fine, but I do not think there is a path forward for actually optimizing based on this assumption by default.

Add an option to toggle this optimization.