Skip to content

[InstCombine] Fold "extract (select (cond, insert(agg, elem), FV))" #115969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

weiguozhi
Copy link
Contributor

Do the transform

     extract (select (cond, insert(agg, elem), FV))
 ->  select (cond, elem, extract(FV))

Do the transform
     extract (select (cond, insert(agg, elem), FV))
 ->  select (cond, elem, extract(FV))
@weiguozhi weiguozhi requested a review from nikic as a code owner November 13, 2024 00:43
@weiguozhi weiguozhi requested review from dtcxzyw and removed request for nikic November 13, 2024 00:43
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Nov 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 13, 2024

@llvm/pr-subscribers-llvm-transforms

Author: None (weiguozhi)

Changes

Do the transform

     extract (select (cond, insert(agg, elem), FV))
 ->  select (cond, elem, extract(FV))

Full diff: https://github.com/llvm/llvm-project/pull/115969.diff

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+19-1)
  • (modified) llvm/test/Transforms/InstCombine/extract-select-agg.ll (+24-7)
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 2a54390c0f1882..5914a7aa422e11 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -1692,7 +1692,25 @@ Instruction *InstCombinerImpl::FoldOpIntoSelect(Instruction &Op, SelectInst *SI,
 
   Value *TV = SI->getTrueValue();
   Value *FV = SI->getFalseValue();
-  if (!(isa<Constant>(TV) || isa<Constant>(FV)))
+  if ((Op.getOpcode() == Instruction::ExtractValue) &&
+      (isa<InsertValueInst>(TV) || isa<InsertValueInst>(FV))) {
+    // extract (select (cond, insert(agg, elem), FV))
+    // -> select (cond, elem, extract(FV))
+    ExtractValueInst *EV = dyn_cast<ExtractValueInst>(&Op);
+    Value *NewTV = simplifyExtractValueInst(TV, EV->getIndices(),
+                                            SQ.getWithInstruction(&Op));
+    Value *NewFV = simplifyExtractValueInst(FV, EV->getIndices(),
+                                            SQ.getWithInstruction(&Op));
+    if (!NewTV && !NewFV)
+      return nullptr;
+    Builder.SetInsertPoint(SI);
+    if (!NewTV)
+      NewTV = Builder.CreateExtractValue(TV, EV->getIndices());
+    if (!NewFV)
+      NewFV = Builder.CreateExtractValue(FV, EV->getIndices());
+    return SelectInst::Create(SI->getCondition(), NewTV, NewFV, "", nullptr,
+                              SI);
+  } else if (!(isa<Constant>(TV) || isa<Constant>(FV)))
     return nullptr;
 
   // Bool selects with constant operands can be folded to logical ops.
diff --git a/llvm/test/Transforms/InstCombine/extract-select-agg.ll b/llvm/test/Transforms/InstCombine/extract-select-agg.ll
index 6ba6b1a575601d..2813870feeb931 100644
--- a/llvm/test/Transforms/InstCombine/extract-select-agg.ll
+++ b/llvm/test/Transforms/InstCombine/extract-select-agg.ll
@@ -56,14 +56,9 @@ define void @test_select_agg_multiuse(i1 %cond, i64 %v1, i64 %v2, i64 %v3, i64 %
 ; CHECK-LABEL: define void @test_select_agg_multiuse(
 ; CHECK-SAME: i1 [[COND:%.*]], i64 [[V1:%.*]], i64 [[V2:%.*]], i64 [[V3:%.*]], i64 [[V4:%.*]]) {
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[A0:%.*]] = insertvalue { i64, i64 } poison, i64 [[V1]], 0
-; CHECK-NEXT:    [[A1:%.*]] = insertvalue { i64, i64 } [[A0]], i64 [[V2]], 1
-; CHECK-NEXT:    [[B0:%.*]] = insertvalue { i64, i64 } poison, i64 [[V3]], 0
-; CHECK-NEXT:    [[B1:%.*]] = insertvalue { i64, i64 } [[B0]], i64 [[V4]], 1
-; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[COND]], { i64, i64 } [[A1]], { i64, i64 } [[B1]]
-; CHECK-NEXT:    [[X:%.*]] = extractvalue { i64, i64 } [[SEL]], 0
+; CHECK-NEXT:    [[X:%.*]] = select i1 [[COND]], i64 [[V1]], i64 [[V3]]
 ; CHECK-NEXT:    call void @use(i64 [[X]])
-; CHECK-NEXT:    [[Y:%.*]] = extractvalue { i64, i64 } [[SEL]], 1
+; CHECK-NEXT:    [[Y:%.*]] = select i1 [[COND]], i64 [[V2]], i64 [[V4]]
 ; CHECK-NEXT:    call void @use(i64 [[Y]])
 ; CHECK-NEXT:    ret void
 ;
@@ -81,3 +76,25 @@ entry:
 }
 
 declare void @use(i64)
+
+define i64 @test_extract_select_insert(ptr %p1, i64 %v) {
+; CHECK-LABEL: define i64 @test_extract_select_insert(
+; CHECK-SAME: ptr [[P1:%.*]], i64 [[V:%.*]]) {
+; CHECK-NEXT:    [[CALL:%.*]] = call { ptr, i64 } @foo()
+; CHECK-NEXT:    [[ELM1:%.*]] = extractvalue { ptr, i64 } [[CALL]], 1
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[ELM1]], [[V]]
+; CHECK-NEXT:    [[TMP1:%.*]] = extractvalue { ptr, i64 } [[CALL]], 1
+; CHECK-NEXT:    [[RES:%.*]] = select i1 [[CMP]], i64 4294967294, i64 [[TMP1]]
+; CHECK-NEXT:    ret i64 [[RES]]
+;
+  %call = call { ptr, i64 } @foo()
+  %elm1 = extractvalue { ptr, i64 } %call, 1
+  %cmp = icmp eq i64 %elm1, %v
+  %fca0 = insertvalue { ptr, i64 } poison, ptr %p1, 0
+  %fca1 = insertvalue { ptr, i64 } %fca0, i64 4294967294, 1
+  %select = select i1 %cmp, { ptr, i64 } %fca1, { ptr, i64 } %call
+  %res = extractvalue { ptr, i64 } %select, 1
+  ret i64 %res
+}
+
+declare { ptr, i64 } @foo()

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG. Please wait for additional approval from other reviewers.

We may generalize to use simplifyInstruction for other instructions in the future.

@nikic
Copy link
Contributor

nikic commented Nov 13, 2024

We may generalize to use simplifyInstruction for other instructions in the future.

I'd rather directly do that than add a special case for extractelement. It's easier to generalize this than not. I did a quick test at #116073 and it doesn't seem to affect compile-time, which would have been my main concern.

@dtcxzyw
Copy link
Member

dtcxzyw commented Nov 14, 2024

We may generalize to use simplifyInstruction for other instructions in the future.

I'd rather directly do that than add a special case for extractelement. It's easier to generalize this than not. I did a quick test at #116073 and it doesn't seem to affect compile-time, which would have been my main concern.

@nikic Is it okay to just keep these negative tests and make this patch NFC? Positive tests have been added in 2ca25ab.

@nikic
Copy link
Contributor

nikic commented Nov 14, 2024

We may generalize to use simplifyInstruction for other instructions in the future.

I'd rather directly do that than add a special case for extractelement. It's easier to generalize this than not. I did a quick test at #116073 and it doesn't seem to affect compile-time, which would have been my main concern.

@nikic Is it okay to just keep these negative tests and make this patch NFC? Positive tests have been added in 2ca25ab.

Sure

nikic pushed a commit that referenced this pull request Nov 15, 2024
nikic added a commit that referenced this pull request Nov 18, 2024
Instead of only trying to constant fold the select arms, try to simplify
them. This subsumes #115969
which implements this for extractvalue only.

This is still fairly limited in that we will usually only call
FoldOpIntoSelect in the first place if we have a constant operand. This
can be relaxed in the future if worthwhile.
@weiguozhi
Copy link
Contributor Author

Deprecate this pr because more general version has been committed in 9a844a3.

@weiguozhi weiguozhi closed this Nov 22, 2024
@weiguozhi weiguozhi deleted the extract-select-insert branch November 27, 2024 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants