[RFC][Attributor] change argument privatization to size-based type selection

[vtjnash]

WARNING: for feedback only on approach and design goals, not yet human-edited to meet quality standards.

The current Attributor design assumes that AllocaInst getAllocatedType has semantic meaning. This is fairly safe currently, based on its use of isDenselyPacked to conservatively reject cases where the semantics of the type would be obviously unsound, but it is suboptimal and also contradictory to the design of Attributor's fixed-point use analysis to be taking this information instead from the definition site. It is also one of very few remaining uses of getAllocatedType in the optimization passes. Once all are gone, we can delete this non-semantic accessor, just like typed-pointers and typed-GEP are being deleted for that same reason, and simplify the IR.

There is probably a few options here, depending on what matters to the ABI and the intent of the pass from the original authors: the simplest option is what I've asked Claude to do here, just pick some reasonable size type that maps adequately to the size and alignment of the object on the current platform. Alternatively, we could use PtrUseAnalysis (with my bugfixes in #179726) to collect all uses of the pointer and then compute an estimated fixed-point partitioning of that information globally (essentially estimating what SROA, where PtrUseAnalysis originally came from, will be able to do once the argument values are split, plus IPO handling)–the benefit of this approach being that several other passes (GlobalOpt and GPU targets) already do, or need to do, that same memory access analysis.

tl;dr: Replace type-based privatization with size-based approach that selects optimal types based on data size, alignment, and target capabilities. Uses integers for small data, vectors for medium sizes, and aligned arrays for larger data.

[github-actions]

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:

git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef([^a-zA-Z0-9_-]|$)|UndefValue::get)' 'HEAD~1' HEAD llvm/include/llvm/Transforms/IPO/Attributor.h llvm/lib/Transforms/IPO/AttributorAttributes.cpp llvm/test/Transforms/Attributor/ArgumentPromotion/X86/attributes.ll llvm/test/Transforms/Attributor/ArgumentPromotion/X86/min-legal-vector-width.ll llvm/test/Transforms/Attributor/ArgumentPromotion/array.ll llvm/test/Transforms/Attributor/ArgumentPromotion/attrs.ll llvm/test/Transforms/Attributor/ArgumentPromotion/byval-2.ll llvm/test/Transforms/Attributor/ArgumentPromotion/byval.ll llvm/test/Transforms/Attributor/ArgumentPromotion/fp80.ll llvm/test/Transforms/Attributor/ArgumentPromotion/tail.ll llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll llvm/test/Transforms/Attributor/nofpclass.ll llvm/test/Transforms/Attributor/value-simplify-pointer-info.ll llvm/test/Transforms/Attributor/value-simplify.ll

The following files introduce new uses of undef:

• llvm/test/Transforms/Attributor/ArgumentPromotion/array.ll

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions cpp,h -- llvm/include/llvm/Transforms/IPO/Attributor.h llvm/lib/Transforms/IPO/AttributorAttributes.cpp --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
index 17b9b9308..26fd1afc7 100644
--- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
+++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
@@ -7382,9 +7382,8 @@ struct AAPrivatizablePtrArgument final : public AAPrivatizablePtrImpl {
     }
 
     // Get alignment information for choosing optimal type
-    const auto *AlignAA =
-        A.getAAFor<AAAlign>(*this, IRPosition::value(getAssociatedValue()),
-                            DepClassTy::OPTIONAL);
+    const auto *AlignAA = A.getAAFor<AAAlign>(
+        *this, IRPosition::value(getAssociatedValue()), DepClassTy::OPTIONAL);
     Align Alignment = AlignAA ? AlignAA->getAssumedAlign() : Align(1);
 
     // Get TTI for target-specific type selection
@@ -7532,9 +7531,9 @@ struct AAPrivatizablePtrArgument final : public AAPrivatizablePtrImpl {
   /// 2. Up to vector register size: use vector of integers
   /// 3. Otherwise: use array of integers sized to alignment
   static Type *getPrivatizableReplacementType(uint64_t Size, Align Alignment,
-                                               const DataLayout &DL,
-                                               LLVMContext &Ctx,
-                                               const TargetTransformInfo *TTI) {
+                                              const DataLayout &DL,
+                                              LLVMContext &Ctx,
+                                              const TargetTransformInfo *TTI) {
     // Strategy 1: If size fits in pointer, use integer type
     uint64_t PointerSizeInBits = DL.getPointerSizeInBits();
     if (Size * 8 <= PointerSizeInBits)
@@ -7574,10 +7573,11 @@ struct AAPrivatizablePtrArgument final : public AAPrivatizablePtrImpl {
     return ArrayType::get(ElementTy, NumElements);
   }
 
-  /// Initialize \p Base with the replacement value argument at position \p ArgNo.
-  /// Stores the value argument into the alloca.
-  static void createInitialization(Type *ReplacementTy, Value &Base, Function &F,
-                                   unsigned ArgNo, BasicBlock::iterator IP) {
+  /// Initialize \p Base with the replacement value argument at position \p
+  /// ArgNo. Stores the value argument into the alloca.
+  static void createInitialization(Type *ReplacementTy, Value &Base,
+                                   Function &F, unsigned ArgNo,
+                                   BasicBlock::iterator IP) {
     assert(ReplacementTy && "Expected valid replacement type!");
 
     // The argument is a value - store it into the alloca
@@ -7603,7 +7603,8 @@ struct AAPrivatizablePtrArgument final : public AAPrivatizablePtrImpl {
   ChangeStatus manifest(Attributor &A) override {
     if (!PrivatizableSize)
       return ChangeStatus::UNCHANGED;
-    assert(!PrivatizableSize->isZero() && "Expected non-zero privatizable size!");
+    assert(!PrivatizableSize->isZero() &&
+           "Expected non-zero privatizable size!");
 
     // Collect all tail calls in the function as we cannot allow new allocas to
     // escape into tail recursion.
@@ -7627,7 +7628,8 @@ struct AAPrivatizablePtrArgument final : public AAPrivatizablePtrImpl {
         A.getAAFor<AAAlign>(*this, IRPosition::value(*Arg), DepClassTy::NONE);
 
     // Callback to repair the associated function. A new alloca is placed at the
-    // beginning and initialized with the replacement value passed as an argument.
+    // beginning and initialized with the replacement value passed as an
+    // argument.
     Type *RepTy = PrivatizableType;
     Attributor::ArgumentReplacementInfo::CalleeRepairCBTy FnRepairCB =
         [=](const Attributor::ArgumentReplacementInfo &ARI,
@@ -7636,10 +7638,10 @@ struct AAPrivatizablePtrArgument final : public AAPrivatizablePtrImpl {
           BasicBlock::iterator IP = EntryBB.getFirstInsertionPt();
           const DataLayout &DL = IP->getDataLayout();
           unsigned AS = DL.getAllocaAddrSpace();
-          Instruction *AI = new AllocaInst(RepTy, AS,
-                                           Arg->getName() + ".priv", IP);
-          createInitialization(RepTy, *AI, ReplacementFn,
-                               ArgIt->getArgNo(), IP);
+          Instruction *AI =
+              new AllocaInst(RepTy, AS, Arg->getName() + ".priv", IP);
+          createInitialization(RepTy, *AI, ReplacementFn, ArgIt->getArgNo(),
+                               IP);
 
           if (AI->getType() != Arg->getType())
             AI = BitCastInst::CreatePointerBitCastOrAddrSpaceCast(
@@ -7650,16 +7652,16 @@ struct AAPrivatizablePtrArgument final : public AAPrivatizablePtrImpl {
             CI->setTailCall(false);
         };
 
-    // Callback to repair a call site of the associated function. The replacement
-    // value is loaded prior to the call and passed to the new function version.
+    // Callback to repair a call site of the associated function. The
+    // replacement value is loaded prior to the call and passed to the new
+    // function version.
     Attributor::ArgumentReplacementInfo::ACSRepairCBTy ACSRepairCB =
         [=](const Attributor::ArgumentReplacementInfo &ARI,
             AbstractCallSite ACS, SmallVectorImpl<Value *> &NewArgOperands) {
           // When no alignment is specified for the load instruction,
           // natural alignment is assumed.
           createReplacementValues(
-              AlignAA ? AlignAA->getAssumedAlign() : Align(0),
-              RepTy, ACS,
+              AlignAA ? AlignAA->getAssumedAlign() : Align(0), RepTy, ACS,
               ACS.getCallArgOperand(ARI.getReplacedArg().getArgNo()),
               NewArgOperands);
         };

[github-actions]

🐧 Linux x64 Test Results

• 3113 tests passed
• 7 tests skipped

All executed tests passed, but another part of the build failed. Click on a failure below to see the details.

lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o

FAILED: lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o
sccache /opt/llvm/bin/clang++ -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GLIBCXX_USE_CXX11_ABI=1 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/gha/actions-runner/_work/llvm-project/llvm-project/build/lib/Transforms/IPO -I/home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/lib/Transforms/IPO -I/home/gha/actions-runner/_work/llvm-project/llvm-project/build/include -I/home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/include -gmlt -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wno-pass-failed -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -std=c++17 -UNDEBUG -fno-exceptions -funwind-tables -fno-rtti -MD -MT lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o -MF lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o.d -o lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/AttributorAttributes.cpp.o -c /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
/home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/lib/Transforms/IPO/AttributorAttributes.cpp:257:15: error: unused function 'constructPointer' [-Werror,-Wunused-function]
257 | static Value *constructPointer(Value *Ptr, int64_t Offset,
|               ^~~~~~~~~~~~~~~~
1 error generated.

If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the infrastructure label.