[CGCall] Annotate pointer argument with alignment

[lebedev.ri]

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?

[efriedma]

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.

[lebedev.ri]

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

In D99791#2667047, @efriedma wrote:

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.

[rjmccall]

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]

@rjmccall thank you for taking a look!

In D99791#2670333, @rjmccall wrote:

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]

In D99791#2672528, @lebedev.ri wrote:

@rjmccall thank you for taking a look!

In D99791#2670333, @rjmccall wrote:

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.

[lebedev.ri]

Add an option to toggle this optimization.

clang/docs/ClangCommandLineReference.rst

 .. option:: -fstrict-overflow, -fno-strict-overflow
 
 .. option:: -fstrict-return, -fno-strict-return
 
 .. option:: -fstrict-vtable-pointers, -fno-strict-vtable-pointers
 
 Enable optimizations based on the strict rules for overwriting polymorphic C++ objects
 
+.. option:: -fstrict-pointer-alignment, -fno-strict-pointer-alignment
+
+Enable optimizations based on the strict rules for pointer alignment
+
 .. option:: -fstruct-path-tbaa, -fno-struct-path-tbaa
 
 .. option:: -fsymbol-partition=<arg>
 
 .. option:: -ftabstop=<arg>
 
 .. option:: -ftemplate-backtrace-limit=<arg>
 

clang/docs/ReleaseNotes.rst

 - Passing underaligned pointers as function arguments is undefined behaviour.
   Previously, clang was not making any optimizations based on that,
   however, there's interest in doing so now.
   UBSan's ``-fsanitize=alignment`` was enhanced to catch the cases where
   an underaligned pointer was passed as a function argument,
   to allow codebases to be cleaned up in preparation for this optimization,
   to avoid miscompiles.
 
+- Clang now assumes that all function pointer arguments are properly aligned,
+  and optimizes on that. A new ``-fstrict-pointer-alignment`` flag was
+  introduces (default on), which guards these new optimizations.
+
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - ...
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------

clang/docs/UsersManual.rst

.. option:: -fwhole-program-vtables
    :doc:`hidden LTO visibility <LTOVisibility>`. Requires ``-flto``.
 
 .. option:: -fforce-emit-vtables
 
    In order to improve devirtualization, forces emitting of vtables even in
    modules where it isn't necessary. It causes more inline virtual functions
    to be emitted.
 
+.. option:: -fstrict-pointer-alignment
+
+   Enable optimizations based on the strict rules for pointer alignment.
+   This currently only affects function pointer arguments, and only controls
+   UBSan behaviour, no optimizations are currently performed based on it.
+
 .. option:: -fno-assume-sane-operator-new
 
    Don't assume that the C++'s new operator is sane.
 
    This option tells the compiler to do not assume that C++'s global
    new operator will always return a pointer that does not alias any
    other pointer when the function returns.
 

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(SanitizeCoverageStackDepth, 1, 0) ///< Enable max stack depth tracing
 CODEGENOPT(SanitizeStats     , 1, 0) ///< Collect statistics for sanitizers.
 CODEGENOPT(SimplifyLibCalls  , 1, 1) ///< Set when -fbuiltin is enabled.
 CODEGENOPT(SoftFloat         , 1, 0) ///< -soft-float.
 CODEGENOPT(SpeculativeLoadHardening, 1, 0) ///< Enable speculative load hardening.
 CODEGENOPT(FineGrainedBitfieldAccesses, 1, 0) ///< Enable fine-grained bitfield accesses.
 CODEGENOPT(StrictEnums       , 1, 0) ///< Optimize based on strict enum definition.
 CODEGENOPT(StrictVTablePointers, 1, 0) ///< Optimize based on the strict vtable pointers
+CODEGENOPT(StrictPointerAlignment, 1, 1) ///< Optimize based on the strict pointer alignment rules
 CODEGENOPT(TimePasses        , 1, 0) ///< Set when -ftime-report or -ftime-report= is enabled.
 CODEGENOPT(TimePassesPerRun  , 1, 0) ///< Set when -ftime-report=per-pass-run is enabled.
 CODEGENOPT(TimeTrace         , 1, 0) ///< Set when -ftime-trace is enabled.
 VALUE_CODEGENOPT(TimeTraceGranularity, 32, 500) ///< Minimum time granularity (in microseconds),
                                                ///< traced by time profiler
 CODEGENOPT(UnrollLoops       , 1, 0) ///< Control whether loops are unrolled.
 CODEGENOPT(RerollLoops       , 1, 0) ///< Control whether loops are rerolled.
 CODEGENOPT(NoUseJumpTables   , 1, 0) ///< Set when -fno-jump-tables is enabled.

clang/include/clang/Driver/Options.td

def fstrict_enums : Flag<["-"], "fstrict-enums">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Enable optimizations based on the strict definition of an enum's "
            "value range">,
   MarshallingInfoFlag<CodeGenOpts<"StrictEnums">>;
 defm strict_vtable_pointers : BoolFOption<"strict-vtable-pointers",
   CodeGenOpts<"StrictVTablePointers">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable optimizations based on the strict rules for"
             " overwriting polymorphic C++ objects">,
   NegFlag<SetFalse>>;
+defm strict_pointer_alignment : BoolFOption<"strict-pointer-alignment",
+  CodeGenOpts<"StrictPointerAlignment">, DefaultFalse,
+  NegFlag<SetFalse, [CC1Option], "Disable">,
+  PosFlag<SetTrue,  [CC1Option], "Enable">,
+  BothFlags<[], " optimizations based on the strict rules for pointer alignmen">>;
 def fstrict_overflow : Flag<["-"], "fstrict-overflow">, Group<f_Group>;
 def fsyntax_only : Flag<["-"], "fsyntax-only">,
   Flags<[NoXarchOption,CoreOption,CC1Option,FC1Option]>, Group<Action_Group>;
 def ftabstop_EQ : Joined<["-"], "ftabstop=">, Group<f_Group>;
 def ftemplate_depth_EQ : Joined<["-"], "ftemplate-depth=">, Group<f_Group>;
 def ftemplate_depth_ : Joined<["-"], "ftemplate-depth-">, Group<f_Group>;
 def ftemplate_backtrace_limit_EQ : Joined<["-"], "ftemplate-backtrace-limit=">,
                                    Group<f_Group>;

clang/lib/CodeGen/CGCall.cpp

if (const auto *RefTy = ParamType->getAs<ReferenceType>()) {
         Attrs.addAttribute(llvm::Attribute::NonNull);
       if (PTy->isObjectType()) {
         llvm::Align Alignment =
             getNaturalPointeeTypeAlignment(ParamType).getAsAlign();
         Attrs.addAlignmentAttr(Alignment);
       }
     }
 
+    if (getCodeGenOpts().StrictPointerAlignment) {
+      if (const auto *PtrTy = ParamType->getAs<PointerType>()) {
+        QualType PTy = PtrTy->getPointeeType();
+        if (PTy->isObjectType()) {
+          llvm::Align Alignment =
+              getNaturalPointeeTypeAlignment(ParamType).getAsAlign();
+          Attrs.addAlignmentAttr(Alignment);
+        }
+      }
+    }
+
     switch (FI.getExtParameterInfo(ArgNo).getABI()) {
     case ParameterABI::Ordinary:
       break;
 
     case ParameterABI::SwiftIndirectResult: {
       // Add 'sret' if we haven't already used it for something, but
       // only if the result is void.
       if (!hasUsedSRet && RetTy->isVoidType()) {

clang/lib/Driver/ToolChains/Clang.cpp

#endif
   if (Args.hasFlag(options::OPT_fstrict_vtable_pointers,
                    options::OPT_fno_strict_vtable_pointers,
                    false))
     CmdArgs.push_back("-fstrict-vtable-pointers");
   if (Args.hasFlag(options::OPT_fforce_emit_vtables,
                    options::OPT_fno_force_emit_vtables,
                    false))
     CmdArgs.push_back("-fforce-emit-vtables");
+  if (Args.hasFlag(options::OPT_fstrict_pointer_alignment,
+                   options::OPT_fno_strict_pointer_alignment,
+                   !RawTriple.isOSDarwin()))
+    CmdArgs.push_back("-fstrict-pointer-alignment");
   if (!Args.hasFlag(options::OPT_foptimize_sibling_calls,
                     options::OPT_fno_optimize_sibling_calls))
     CmdArgs.push_back("-mdisable-tail-calls");
   if (Args.hasFlag(options::OPT_fno_escaping_block_tail_calls,
                    options::OPT_fescaping_block_tail_calls, false))
     CmdArgs.push_back("-fno-escaping-block-tail-calls");
 
   Args.AddLastArg(CmdArgs, options::OPT_ffine_grained_bitfield_accesses,