[ArgPromotion] Change the condition to check the promotion limit

[psamolysov]

Summary

The condition should be 'ArgParts.size() > MaxElements', so that if we
have exactly 3 elements in the 'ArgParts' vector, the promotion should
be allowed because the 'MaxElement' threshold is not exceeded yet.

The default value for 'MaxElement' has been decreased to 2 in order
to avoid an actual change in argument promoting behavior. However,
this changes byval argument transformation behavior by allowing
adding not more than 2 arguments to the function instead of 3 allowed
before.

[aeubanks]

looks good

llvm/test/Transforms/ArgumentPromotion/max-elements-limit.ll

2

-o - should be unnecessary

53

remove unrelated IR (and things like dso_local above)

[psamolysov]

@aeubanks Thank you for the comments. I've applied your suggestions.

[psamolysov]

Colleagues, could you help me with the landing, please?

[nikic]

I think we also need to adjust MaxElements by one to avoid an actual change in behavior.

[psamolysov]

@nikic Thank you, good idea. I've adjusted the default value to 2. The LIT test was also updated to cover both cases: if there are 2 parts, the arguments can be promoted but if there are 3 parts, they cannot.

[psamolysov]

Adjust the default value for MaxElements.

[psamolysov]

Rebasing to main, the previous patch cannot be applied by some unknown reasons.

[psamolysov]

Fix the issue with patch applying.

[aeubanks]

llvm/include/llvm/Transforms/IPO/ArgumentPromotion.h

28 ↗

(On Diff #424503)

MaxElements is also used elsewhere (handling byval args) besides the one place below.

I think making the two uses consistent makes sense and should be ok, but that should be noted in the description (this doesn't preserve existing behavior as currently stated). if somebody complains about this regressing anything, we can split MaxElement into two separate parameters

[psamolysov]

llvm/include/llvm/Transforms/IPO/ArgumentPromotion.h

28 ↗

(On Diff #424503)

@aeubanks Thank you for the comment. I've changed the description to make it more concrete, the changing of the default MaxElement value preserves existing behavior for argument promoting but changes behavior for byval argument transformation.

[psamolysov]

If the changes make sense, could you land it, please?

[psamolysov]

Rebase to main

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

auto HandleLoad = [&](LoadInst *LI,
     int64_t Off = Offset.getSExtValue();
     auto Pair = ArgParts.try_emplace(
         Off, ArgPart{Ty, LI->getAlign(), GuaranteedToExecute ? LI : nullptr});
     ArgPart &Part = Pair.first->second;
     bool OffsetNotSeenBefore = Pair.second;
 
     // We limit promotion to only promoting up to a fixed number of elements of
     // the aggregate.
-    if (MaxElements > 0 && ArgParts.size() >= MaxElements) {
+    if (MaxElements > 0 && ArgParts.size() > MaxElements) {
       LLVM_DEBUG(dbgs() << "ArgPromotion of " << *Arg << " failed: "
                         << "more than " << MaxElements << " parts\n");
       return false;
     }
 
     // For now, we only support loading one specific type at a given offset.
     if (Part.Ty != Ty) {
       LLVM_DEBUG(dbgs() << "ArgPromotion of " << *Arg << " failed: "

llvm/test/Transforms/ArgumentPromotion/max-elements-limit.ll

+; RUN: opt -passes=argpromotion -S %s | FileCheck %s
+
+define internal i32 @callee(i32* noundef %0) {
+; CHECK-LABEL: define {{[^@]+}}@callee
+; CHECK-SAME: (i32 [[P_0:%.*]], i32 [[P_1:%.*]], i32 [[P_2:%.*]]) {
+; CHECK-NEXT:    [[SUM_1:%.*]] = add nsw i32 [[P_0]], [[P_1]]
+; CHECK-NEXT:    [[SUM_2:%.*]] = add nsw i32 [[SUM_1]], [[P_2]]
+; CHECK-NEXT:    ret i32 [[SUM_2]]
+;
+  %2 = getelementptr inbounds i32, i32* %0, i64 0
+  %3 = load i32, i32* %2, align 4
+  %4 = getelementptr inbounds i32, i32* %0, i64 1
+  %5 = load i32, i32* %4, align 4
+  %6 = add nsw i32 %3, %5
+  %7 = getelementptr inbounds i32, i32* %0, i64 2
+  %8 = load i32, i32* %7, align 4
+  %9 = add nsw i32 %6, %8
+  ret i32 %9
+}
+
+define i32 @caller(i32 %0, i32 %1, i32 %2) {
+; CHECK-LABEL: define {{[^@]+}}@caller
+; CHECK-SAME: (i32 [[P_0:%.*]], i32 [[P_1:%.*]], i32 [[P_2:%.*]]) {
+; CHECK-NEXT:    [[TMP1:%.*]] = alloca [3 x i32], align 4
+; CHECK-NEXT:    [[PL_0:%.*]] = getelementptr inbounds [3 x i32], [3 x i32]* [[TMP1]], i64 0, i64 0
+; CHECK-NEXT:    store i32 [[P_0]], i32* [[PL_0]], align 4
+; CHECK-NEXT:    [[PL_1:%.*]] = getelementptr inbounds i32, i32* [[PL_0]], i64 1
+; CHECK-NEXT:    store i32 [[P_1]], i32* [[PL_1]], align 4
+; CHECK-NEXT:    [[PL_2:%.*]] = getelementptr inbounds i32, i32* [[PL_1]], i64 1
+; CHECK-NEXT:    store i32 [[P_2]], i32* [[PL_2]], align 4
+; CHECK-NEXT:    [[PL_3:%.*]] = getelementptr inbounds [3 x i32], [3 x i32]* [[TMP1]], i64 0, i64 0
+; CHECK-NEXT:    [[VAL_0:%.*]] = load i32, i32* [[PL_3]], align 4
+; CHECK-NEXT:    [[PL_4:%.*]] = getelementptr i32, i32* [[PL_3]], i64 1
+; CHECK-NEXT:    [[VAL_1:%.*]] = load i32, i32* [[PL_4]], align 4
+; CHECK-NEXT:    [[PL_5:%.*]] = getelementptr i32, i32* [[PL_3]], i64 2
+; CHECK-NEXT:    [[VAL_2:%.*]] = load i32, i32* [[PL_5]], align 4
+; CHECK-NEXT:    [[RES:%.*]] = call i32 @callee(i32 [[VAL_0]], i32 [[VAL_1]], i32 [[VAL_2]])
+; CHECK-NEXT:    ret i32 [[RES]]
+;
+  %4 = alloca [3 x i32], align 4
+  %5 = getelementptr inbounds [3 x i32], [3 x i32]* %4, i64 0, i64 0
+  store i32 %0, i32* %5, align 4
+  %6 = getelementptr inbounds i32, i32* %5, i64 1
+  store i32 %1, i32* %6, align 4
+  %7 = getelementptr inbounds i32, i32* %6, i64 1
+  store i32 %2, i32* %7, align 4
+  %8 = getelementptr inbounds [3 x i32], [3 x i32]* %4, i64 0, i64 0
+  %9 = call i32 @callee(i32* noundef %8)
+  ret i32 %9
+}