[llvm][CodeGen] Added a check in CodeGenPrepare::optimizeSwitchType #83322

karouzakisp · 2024-02-28T19:35:42Z

When we have two or more users in the Switch Condition and we have more SExt Users than ZExt Users or more signed cmp Instruction Users than unsigned cmp Instruction Users, we need to check to not insert a ZExt into the switch condition Type and rather insert a SExt. This way we can eliminate one Instruction.

github-actions · 2024-02-28T19:36:03Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

github-actions · 2024-02-28T19:38:04Z

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

llvmbot · 2024-02-29T14:04:58Z

@llvm/pr-subscribers-backend-risc-v

Author: Panagiotis K (karouzakisp)

Changes

When we have two users in the Switch Condition and one of them is sext we need to check to not insert a sext and not a zext into the switch condition Type. This creates an additional instruction on RISC-V

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

2 Files Affected:

(modified) llvm/lib/CodeGen/CodeGenPrepare.cpp (+13-1)
(modified) llvm/test/CodeGen/RISCV/switch-width.ll (+95-12)

diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index feefe87f406365..82b02fa6216306 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -7397,7 +7397,19 @@ bool CodeGenPrepare::optimizeSwitchType(SwitchInst *SI) {
     if (Arg->hasZExtAttr())
       ExtType = Instruction::ZExt;
   }
-
+  // If Cond has 2 users and one of them is a sext don't use a zext.
+  bool HaveSExtUser = false;
+  if (Cond->hasNUses(2)) {
+    for (auto &U : Cond->uses()) {
+      Value *V = U.get();
+      if (isa<SExtInst>(V)) {
+        HaveSExtUser = true;
+      }
+    }
+  }
+  if (HaveSExtUser) {
+    ExtType = Instruction::SExt;
+  }
   auto *ExtInst = CastInst::Create(ExtType, Cond, NewType);
   ExtInst->insertBefore(SI);
   ExtInst->setDebugLoc(SI->getDebugLoc());
diff --git a/llvm/test/CodeGen/RISCV/switch-width.ll b/llvm/test/CodeGen/RISCV/switch-width.ll
index d902bd3276a3ce..fa3a321d85c471 100644
--- a/llvm/test/CodeGen/RISCV/switch-width.ll
+++ b/llvm/test/CodeGen/RISCV/switch-width.ll
@@ -194,7 +194,90 @@ return:
   %retval = phi i32 [ -1, %sw.default ], [ 0, %sw.bb0 ], [ 1, %sw.bb1 ]
   ret i32 %retval
 }
+define i32 @sext_i32(i16 %a)  {
+; CHECK-LABEL: sext_i32:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    lui a1, 16
+; CHECK-NEXT:    addiw a1, a1, -1
+; CHECK-NEXT:    and a2, a0, a1
+; CHECK-NEXT:    beq a2, a1, .LBB5_3
+; CHECK-NEXT:  # %bb.1: # %entry
+; CHECK-NEXT:    slli a0, a0, 48
+; CHECK-NEXT:    srai a0, a0, 48
+; CHECK-NEXT:    li a1, 1
+; CHECK-NEXT:    bne a0, a1, .LBB5_4
+; CHECK-NEXT:  # %bb.2: # %sw.bb0
+; CHECK-NEXT:    li a0, 0
+; CHECK-NEXT:    ret
+; CHECK-NEXT:  .LBB5_3: # %sw.bb1
+; CHECK-NEXT:    li a0, 1
+; CHECK-NEXT:    ret
+; CHECK-NEXT:  .LBB5_4: # %sw.default
+; CHECK-NEXT:    li a0, -1
+; CHECK-NEXT:    ret
+entry:
+  %sext = sext i16 %a to i32
+  switch i32 %sext, label %sw.default [
+  i32 1, label %sw.bb0
+  i32 -1, label %sw.bb1
+  ]
+
+sw.bb0:
+  br label %return
+
+sw.bb1:
+  br label %return
+
+sw.default:
+  br label %return
+
+return:
+  %retval = phi i32 [ -1, %sw.default ], [ 0, %sw.bb0 ], [ 1, %sw.bb1 ]
+  ret i32 %retval
+}
+
+define i32 @sext_i16(i8 %a)  {
+; CHECK-LABEL: sext_i16:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    andi a1, a0, 255
+; CHECK-NEXT:    li a2, 255
+; CHECK-NEXT:    beq a1, a2, .LBB6_3
+; CHECK-NEXT:  # %bb.1: # %entry
+; CHECK-NEXT:    slli a0, a0, 56
+; CHECK-NEXT:    srai a0, a0, 56
+; CHECK-NEXT:    slli a0, a0, 48
+; CHECK-NEXT:    srli a0, a0, 48
+; CHECK-NEXT:    li a1, 1
+; CHECK-NEXT:    bne a0, a1, .LBB6_4
+; CHECK-NEXT:  # %bb.2: # %sw.bb0
+; CHECK-NEXT:    li a0, 0
+; CHECK-NEXT:    ret
+; CHECK-NEXT:  .LBB6_3: # %sw.bb1
+; CHECK-NEXT:    li a0, 1
+; CHECK-NEXT:    ret
+; CHECK-NEXT:  .LBB6_4: # %sw.default
+; CHECK-NEXT:    li a0, -1
+; CHECK-NEXT:    ret
+entry:
+  %sext = sext i8 %a to i16
+  switch i16 %sext, label %sw.default [
+  i16 1, label %sw.bb0
+  i16 -1, label %sw.bb1
+  ]
+
+sw.bb0:
+  br label %return
 
+sw.bb1:
+  br label %return
+
+sw.default:
+  br label %return
+
+return:
+  %retval = phi i32 [ -1, %sw.default ], [ 0, %sw.bb0 ], [ 1, %sw.bb1 ]
+  ret i32 %retval
+}
 
 define i32 @trunc_i12(i64 %a)  {
 ; CHECK-LABEL: trunc_i12:
@@ -202,17 +285,17 @@ define i32 @trunc_i12(i64 %a)  {
 ; CHECK-NEXT:    lui a1, 1
 ; CHECK-NEXT:    addiw a1, a1, -1
 ; CHECK-NEXT:    and a0, a0, a1
-; CHECK-NEXT:    beq a0, a1, .LBB5_3
+; CHECK-NEXT:    beq a0, a1, .LBB7_3
 ; CHECK-NEXT:  # %bb.1: # %entry
 ; CHECK-NEXT:    li a1, 1
-; CHECK-NEXT:    bne a0, a1, .LBB5_4
+; CHECK-NEXT:    bne a0, a1, .LBB7_4
 ; CHECK-NEXT:  # %bb.2: # %sw.bb0
 ; CHECK-NEXT:    li a0, 0
 ; CHECK-NEXT:    ret
-; CHECK-NEXT:  .LBB5_3: # %sw.bb1
+; CHECK-NEXT:  .LBB7_3: # %sw.bb1
 ; CHECK-NEXT:    li a0, 1
 ; CHECK-NEXT:    ret
-; CHECK-NEXT:  .LBB5_4: # %sw.default
+; CHECK-NEXT:  .LBB7_4: # %sw.default
 ; CHECK-NEXT:    li a0, -1
 ; CHECK-NEXT:    ret
 entry:
@@ -241,17 +324,17 @@ define i32 @trunc_i11(i64 %a)  {
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    andi a0, a0, 2047
 ; CHECK-NEXT:    li a1, 2047
-; CHECK-NEXT:    beq a0, a1, .LBB6_3
+; CHECK-NEXT:    beq a0, a1, .LBB8_3
 ; CHECK-NEXT:  # %bb.1: # %entry
 ; CHECK-NEXT:    li a1, 1
-; CHECK-NEXT:    bne a0, a1, .LBB6_4
+; CHECK-NEXT:    bne a0, a1, .LBB8_4
 ; CHECK-NEXT:  # %bb.2: # %sw.bb0
 ; CHECK-NEXT:    li a0, 0
 ; CHECK-NEXT:    ret
-; CHECK-NEXT:  .LBB6_3: # %sw.bb1
+; CHECK-NEXT:  .LBB8_3: # %sw.bb1
 ; CHECK-NEXT:    li a0, 1
 ; CHECK-NEXT:    ret
-; CHECK-NEXT:  .LBB6_4: # %sw.default
+; CHECK-NEXT:  .LBB8_4: # %sw.default
 ; CHECK-NEXT:    li a0, -1
 ; CHECK-NEXT:    ret
 entry:
@@ -281,17 +364,17 @@ define i32 @trunc_i10(i64 %a)  {
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    andi a0, a0, 1023
 ; CHECK-NEXT:    li a1, 1023
-; CHECK-NEXT:    beq a0, a1, .LBB7_3
+; CHECK-NEXT:    beq a0, a1, .LBB9_3
 ; CHECK-NEXT:  # %bb.1: # %entry
 ; CHECK-NEXT:    li a1, 1
-; CHECK-NEXT:    bne a0, a1, .LBB7_4
+; CHECK-NEXT:    bne a0, a1, .LBB9_4
 ; CHECK-NEXT:  # %bb.2: # %sw.bb0
 ; CHECK-NEXT:    li a0, 0
 ; CHECK-NEXT:    ret
-; CHECK-NEXT:  .LBB7_3: # %sw.bb1
+; CHECK-NEXT:  .LBB9_3: # %sw.bb1
 ; CHECK-NEXT:    li a0, 1
 ; CHECK-NEXT:    ret
-; CHECK-NEXT:  .LBB7_4: # %sw.default
+; CHECK-NEXT:  .LBB9_4: # %sw.default
 ; CHECK-NEXT:    li a0, -1
 ; CHECK-NEXT:    ret
 entry:

arsenm · 2024-02-29T14:05:07Z

Can you rephrase the description to say what the check is, and why?

preames · 2024-02-29T21:21:58Z

llvm/lib/CodeGen/CodeGenPrepare.cpp

@@ -7397,7 +7397,19 @@ bool CodeGenPrepare::optimizeSwitchType(SwitchInst *SI) {
    if (Arg->hasZExtAttr())
      ExtType = Instruction::ZExt;
  }
-
+  // If Cond has 2 users and one of them is a sext don't use a zext.


We should probably be looking at the other user and using it's extend kind (sign or zero).

we probably do want the argument extend kind check to overrule, so maybe put this just above?

preames · 2024-02-29T21:23:32Z

llvm/test/CodeGen/RISCV/switch-width.ll

@@ -194,25 +194,108 @@ return:
  %retval = phi i32 [ -1, %sw.default ], [ 0, %sw.bb0 ], [ 1, %sw.bb1 ]
  ret i32 %retval
 }
+define i32 @sext_i32(i16 %a)  {


You should add test variants here with zeroext and signext on the arguments to make sure we test the interaction.

so that if we have more SExt Users than ZExt Users we use SExt instead of a ZExt in the Switch Condition because that creates a redundant instruction.

arsenm · 2024-03-27T08:47:42Z

llvm/lib/CodeGen/CodeGenPrepare.cpp

  }

+  // Some targets prefer SExt over ZExt.
+  if (TLI->isSExtCheaperThanZExt(OldVT, RegType))


What was the point of doing this user scan if you just throw away the result?

Because in some cases we have more ZExtUsers but SExt is cheaper so if create a ZExt we add more instructions.
I checked spec2017 for RISC-V with the precedence with this and It was doing worse on 500.perlbench_r if we check SExtIsCheaperThanZExt before the user scan. I can share the results later.

This is the text sizes with SExtIsCheaperThanZExt take no precedence over the user scan

And this is the text sizes with SExtIsCheaperThanZExt taking precedence than user scan.

I don't mean the end result, I mean the computation you perform in this code is discarded based on isSExtCheaperThanZExt

Yes isSExtCheaperThanZExt needs to take precedence if it doesn't agree with the computation that I perform otherwise the code size increases as I showed above.
Maybe the gain is very small compared to the cost because we need a User Scan.

My reading of @arsenm's comment is that maybe it would be better to avoid the user scan if we dont need it as such:

if (TLI->isSExtCheaperThanZExt(OldVT, RegType)) ExtType = Instruction::SExt; else { // Move user scan above to here }

If one of the users is a ZExt, then ZExt should be preferable to SExt even if SExt is cheaper. An unsigned ICmp should maybe be treated as SExtUser if isSExtCheaperThanZExt. Type legalization will use sext instead of zext for unsigned icmps when isSExtCheaperThanZExt is true.

arsenm

Title should be more specific about the purpose of the check, rather than state that a check was added

arsenm · 2024-04-23T12:06:12Z

llvm/lib/CodeGen/CodeGenPrepare.cpp

  }

+  // Some targets prefer SExt over ZExt.
+  if (TLI->isSExtCheaperThanZExt(OldVT, RegType))


I don't mean the end result, I mean the computation you perform in this code is discarded based on isSExtCheaperThanZExt

arsenm

Title needs improvement

karouzakisp force-pushed the fix_codegenPrepare_switch_zext branch 2 times, most recently from b3302a5 to c680234 Compare February 29, 2024 00:05

arsenm added the backend:RISC-V label Feb 29, 2024

arsenm requested review from asb, preames and topperc February 29, 2024 14:04

preames requested changes Feb 29, 2024

View reviewed changes

karouzakisp force-pushed the fix_codegenPrepare_switch_zext branch 2 times, most recently from 31888ac to 7dd7633 Compare March 27, 2024 04:01

[llvm][CodeGen] Added a check in CodeGenPrepare::OptimizeSwitchType,

c186cf5

so that if we have more SExt Users than ZExt Users we use SExt instead of a ZExt in the Switch Condition because that creates a redundant instruction.

karouzakisp force-pushed the fix_codegenPrepare_switch_zext branch from 7dd7633 to c186cf5 Compare March 27, 2024 04:18

arsenm reviewed Mar 27, 2024

View reviewed changes

arsenm reviewed Apr 23, 2024

View reviewed changes

arsenm requested changes May 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm][CodeGen] Added a check in CodeGenPrepare::optimizeSwitchType #83322

[llvm][CodeGen] Added a check in CodeGenPrepare::optimizeSwitchType #83322

karouzakisp commented Feb 28, 2024 •

edited

github-actions bot commented Feb 28, 2024

github-actions bot commented Feb 28, 2024 •

edited

llvmbot commented Feb 29, 2024

arsenm commented Feb 29, 2024

preames Feb 29, 2024

preames Feb 29, 2024

arsenm Mar 27, 2024

karouzakisp Mar 27, 2024 •

edited

karouzakisp Mar 27, 2024

arsenm Apr 23, 2024

karouzakisp Apr 23, 2024

michaelmaitland May 20, 2024 •

edited

topperc May 20, 2024 •

edited

arsenm left a comment

arsenm Apr 23, 2024

arsenm left a comment

[llvm][CodeGen] Added a check in CodeGenPrepare::optimizeSwitchType #83322

Are you sure you want to change the base?

[llvm][CodeGen] Added a check in CodeGenPrepare::optimizeSwitchType #83322

Conversation

karouzakisp commented Feb 28, 2024 • edited

github-actions bot commented Feb 28, 2024

github-actions bot commented Feb 28, 2024 • edited

llvmbot commented Feb 29, 2024

arsenm commented Feb 29, 2024

preames Feb 29, 2024

Choose a reason for hiding this comment

preames Feb 29, 2024

Choose a reason for hiding this comment

arsenm Mar 27, 2024

Choose a reason for hiding this comment

karouzakisp Mar 27, 2024 • edited

Choose a reason for hiding this comment

karouzakisp Mar 27, 2024

Choose a reason for hiding this comment

arsenm Apr 23, 2024

Choose a reason for hiding this comment

karouzakisp Apr 23, 2024

Choose a reason for hiding this comment

michaelmaitland May 20, 2024 • edited

Choose a reason for hiding this comment

topperc May 20, 2024 • edited

Choose a reason for hiding this comment

arsenm left a comment

Choose a reason for hiding this comment

arsenm Apr 23, 2024

Choose a reason for hiding this comment

arsenm left a comment

Choose a reason for hiding this comment

karouzakisp commented Feb 28, 2024 •

edited

github-actions bot commented Feb 28, 2024 •

edited

karouzakisp Mar 27, 2024 •

edited

michaelmaitland May 20, 2024 •

edited

topperc May 20, 2024 •

edited