[SelectionDAG] Simplify vselect true, T, F -> T #100992

lukel97 · 2024-07-29T09:18:44Z

This addresses a TODO where we can fold a vselect to it's true operand if the boolean is known to be all trues, by factoring out the logic from extractBooleanFlip which checks TLI.getBooleanContents.

This addresses a TODO where we can fold a vselect to it's true operand if the boolean is known to be all trues, checking getBooleanContents.

llvmbot · 2024-07-29T09:19:19Z

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-selectiondag

Author: Luke Lau (lukel97)

Changes

This addresses a TODO where we can fold a vselect to it's true operand if the boolean is known to be all trues, checking getBooleanContents.

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

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+17-3)
(added) llvm/test/CodeGen/RISCV/rvv/vp-select.ll (+10)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index daebcdabda984..1876bf3f18c21 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -9927,13 +9927,27 @@ SDValue SelectionDAG::simplifySelect(SDValue Cond, SDValue T, SDValue F) {
   if (auto *CondC = dyn_cast<ConstantSDNode>(Cond))
     return CondC->isZero() ? F : T;
 
-  // TODO: This should simplify VSELECT with non-zero constant condition using
-  // something like this (but check boolean contents to be complete?):
   if (ConstantSDNode *CondC = isConstOrConstSplat(Cond, /*AllowUndefs*/ false,
-                                                  /*AllowTruncation*/ true))
+                                                  /*AllowTruncation*/ true)) {
     if (CondC->isZero())
       return F;
 
+    switch (TLI->getBooleanContents(Cond.getValueType())) {
+    case TargetLowering::UndefinedBooleanContent:
+      if (CondC->getAPIntValue()[0])
+        return T;
+      break;
+    case TargetLowering::ZeroOrOneBooleanContent:
+      if (CondC->isOne())
+        return T;
+      break;
+    case TargetLowering::ZeroOrNegativeOneBooleanContent:
+      if (CondC->isAllOnes())
+        return T;
+      break;
+    }
+  }
+
   // select ?, T, T --> T
   if (T == F)
     return T;
diff --git a/llvm/test/CodeGen/RISCV/rvv/vp-select.ll b/llvm/test/CodeGen/RISCV/rvv/vp-select.ll
new file mode 100644
index 0000000000000..5c903791927c6
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/rvv/vp-select.ll
@@ -0,0 +1,10 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=riscv64 -mattr=+v -verify-machineinstrs | FileCheck %s
+
+define <vscale x 1 x i64> @all_ones(<vscale x 1 x i64> %true, <vscale x 1 x i64> %false, i32 %evl) {
+; CHECK-LABEL: all_ones:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    ret
+  %v = call <vscale x 1 x i64> @llvm.vp.select.nxv1i64(<vscale x 1 x i1> splat (i1 true), <vscale x 1 x i64> %true, <vscale x 1 x i64> %false, i32 %evl)
+  ret <vscale x 1 x i64> %v
+}

RKSimon · 2024-07-29T09:35:21Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

+    case TargetLowering::ZeroOrNegativeOneBooleanContent:
+      if (CondC->isAllOnes())
+        return T;
+      break;


Not sure if its worth it but we already do this in at least one other place I can recall (extractBooleanFlip) - we could add a SelectionDAG::isBoolConstant(SDValue Cond, bool V) helper to handle this?

Might as well whilst we're here, will do

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

arsenm · 2024-07-29T10:28:27Z

llvm/test/CodeGen/RISCV/rvv/vp-select.ll

@@ -0,0 +1,10 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=riscv64 -mattr=+v -verify-machineinstrs | FileCheck %s


Suggested change

; RUN: llc < %s -mtriple=riscv64 -mattr=+v -verify-machineinstrs | FileCheck %s

; RUN: llc -mtriple=riscv64 -mattr=+v < %s | FileCheck %s

Don't need to add -verify-machineinstrs in any random test

I specifically added it for this test since it's dealing with RVV instructions. After we migrated the RISCVInsertVSETVLI pass to operate on LiveIntervals it really helps to have the machine verifier check that they're intact. It's already caught quite a few issues just on the existing RVV codegen tests alone.

They'll get caught in expensive_checks builds. Unless this test is specifically trying to stress a verifier error, which it isn't, it's just wasting test time

This is already the de-facto for the RVV in-tree tests:

$ git grep "\-verify\-machineinstrs" -- llvm/test/CodeGen/RISCV/rvv | wc -l 2017

I get your point about slow test times but we rely very heavily on this for coverage on RISCVInsertVSETVLI. Should we create a separate issue to discuss this outside of this PR

Right I think this is a long standing issue, and we have thousands of violations. It defeats the point of having a separate EXPENSIVE_CHECKS behavior

Right I think this is a long standing issue, and we have thousands of violations. It defeats the point of having a separate EXPENSIVE_CHECKS behavior

Do we have a RISCV expensive checks bot?

Wouldn't the x86_64 expensive checks bot catch this? https://lab.llvm.org/buildbot/#/builders/16

Wouldn't the x86_64 expensive checks bot catch this? https://lab.llvm.org/buildbot/#/builders/16

I think it would. I just wasn't sure what targets it builds.

Created #102092 to separate out this discussion

arsenm · 2024-07-29T10:28:51Z

llvm/test/CodeGen/RISCV/rvv/vp-select.ll

+; CHECK-LABEL: all_ones:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    ret
+  %v = call <vscale x 1 x i64> @llvm.vp.select.nxv1i64(<vscale x 1 x i1> splat (i1 true), <vscale x 1 x i64> %true, <vscale x 1 x i64> %false, i32 %evl)


Also test the all false case? Plus another target with the other bits behavior?

I tried creating a test case on r600 which seems to have the different behaviour but couldn't come up with anything meaningful. Mainly because the combine kicks in when the condition vector is at nxi1, so every behaviour considers it true.

github-actions · 2024-07-29T11:09:38Z

✅ With the latest revision this PR passed the C/C++ code formatter.

RKSimon · 2024-07-29T13:29:42Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

+  if (!Const)
+    return std::nullopt;
+
+  switch (TLI->getBooleanContents(N.getValueType())) {


It might be safer if we only accept the 'clean' 1/0, -1/0 and 1b/0b T/F values and std::nullopt otherwise.

Ah I see what you meant by the original signature you suggested, that makes sense

Thanks - an alternative would be an optional result that returns the exact match - which would mean we only need one call in simplifySelect. No strong preference on this one though tbh - as long as we only accept the 'clean' boolean contents values.

…r if operand isn't constant, update tests on other targets

lukel97 · 2024-08-05T04:11:08Z

Gentle ping

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

RKSimon · 2024-08-05T11:19:24Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

+  if (!Const)
+    return std::nullopt;
+
+  switch (TLI->getBooleanContents(N.getValueType())) {


Thanks - an alternative would be an optional result that returns the exact match - which would mean we only need one call in simplifySelect. No strong preference on this one though tbh - as long as we only accept the 'clean' boolean contents values.

…ptional

RKSimon

LGTM - the riscv-gurus still need to confirm what to do regarding using verify-machineinstrs in rvv tests

lukel97 added 2 commits July 29, 2024 17:02

Precommit tests

52d1103

[SelectionDAG] Simplify vselect true, T, F -> T

4efe9db

This addresses a TODO where we can fold a vselect to it's true operand if the boolean is known to be all trues, checking getBooleanContents.

lukel97 requested review from Arsenic-ATG, arsenm, preames and RKSimon and removed request for Arsenic-ATG July 29, 2024 09:18

llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Jul 29, 2024

RKSimon reviewed Jul 29, 2024

View reviewed changes

Extract out isBoolConstant

7417903

arsenm reviewed Jul 29, 2024

View reviewed changes

lukel97 added 2 commits July 29, 2024 19:05

Add all false test

15400c1

Add equals to comment

4552d30

clang-format

4eee8f5

RKSimon reviewed Jul 29, 2024

View reviewed changes

Add Valuev arg to isBoolConstant, restore extractBooleanFlip behaviou…

4de34ee

…r if operand isn't constant, update tests on other targets

llvmbot added backend:AArch64 backend:X86 labels Jul 29, 2024

RKSimon reviewed Aug 5, 2024

View reviewed changes

Move isConstSplat check to Force, change isBoolConstant to use std::o…

64f52d9

…ptional

RKSimon approved these changes Aug 5, 2024

View reviewed changes

lukel97 mentioned this pull request Aug 6, 2024

Should we remove unnecessary uses of -verify-machineinstrs in test/CodeGen? #102092

Open

lukel97 merged commit 33fc322 into llvm:main Aug 6, 2024
7 checks passed

		@@ -0,0 +1,10 @@
		; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
		; RUN: llc < %s -mtriple=riscv64 -mattr=+v -verify-machineinstrs \| FileCheck %s

	; RUN: llc < %s -mtriple=riscv64 -mattr=+v -verify-machineinstrs \| FileCheck %s
	; RUN: llc -mtriple=riscv64 -mattr=+v < %s \| FileCheck %s

[SelectionDAG] Simplify vselect true, T, F -> T #100992

[SelectionDAG] Simplify vselect true, T, F -> T #100992

Uh oh!

Conversation

lukel97 commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukel97 commented Aug 5, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lukel97 commented Jul 29, 2024 •

edited

Loading

llvmbot commented Jul 29, 2024 •

edited

Loading

github-actions bot commented Jul 29, 2024 •

edited

Loading