[IndVars] Teach widenLoopCompare to use sext if narrow IV is positive and other operand is already sext. #142703

topperc · 2025-06-04T00:25:02Z

This prevents us from ending up with (zext (sext X)). The zext will
require an instruction on targets where zext isn't free like RISC-V.

… and other operand is already sext. This prevents us from ending up with (zext (sext X)). The zext will require an instruction on targets where zext isn't free like RISC-V.

llvmbot · 2025-06-04T00:25:42Z

@llvm/pr-subscribers-llvm-transforms

Author: Craig Topper (topperc)

Changes

This prevents us from ending up with (zext (sext X)). The zext will
require an instruction on targets where zext isn't free like RISC-V.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Utils/SimplifyIndVar.cpp (+6)
(added) llvm/test/Transforms/IndVarSimplify/iv-cmp-sext.ll (+66)

diff --git a/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp b/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
index 3fe4621eca70d..6d2266a5a98b5 100644
--- a/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
@@ -1630,6 +1630,12 @@ bool WidenIV::widenLoopCompare(WidenIV::NarrowIVDefUse DU) {
 
   // Widen the other operand of the compare, if necessary.
   if (CastWidth < IVWidth) {
+    // If the narrow IV is always postive and the other operand is sext, widen
+    // using sext so we can combine them. This works for all comparison
+    // predicates.
+    if (DU.NeverNegative && isa<SExtInst>(Op))
+      CmpPreferredSign = true;
+
     Value *ExtOp = createExtendInst(Op, WideType, CmpPreferredSign, Cmp);
     DU.NarrowUse->replaceUsesOfWith(Op, ExtOp);
   }
diff --git a/llvm/test/Transforms/IndVarSimplify/iv-cmp-sext.ll b/llvm/test/Transforms/IndVarSimplify/iv-cmp-sext.ll
new file mode 100644
index 0000000000000..34925a6cca955
--- /dev/null
+++ b/llvm/test/Transforms/IndVarSimplify/iv-cmp-sext.ll
@@ -0,0 +1,66 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -passes=indvars -S | FileCheck %s
+
+target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128"
+target triple = "riscv64"
+
+define void @foo(ptr %x, i32 %n) {
+; CHECK-LABEL: define void @foo(
+; CHECK-SAME: ptr [[X:%.*]], i32 [[N:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[CMP10:%.*]] = icmp sgt i32 [[N]], 0
+; CHECK-NEXT:    br i1 [[CMP10]], label %[[FOR_BODY_PREHEADER:.*]], label %[[FOR_COND_CLEANUP:.*]]
+; CHECK:       [[FOR_BODY_PREHEADER]]:
+; CHECK-NEXT:    [[WIDE_TRIP_COUNT:%.*]] = zext i32 [[N]] to i64
+; CHECK-NEXT:    br label %[[FOR_BODY:.*]]
+; CHECK:       [[FOR_COND_CLEANUP_LOOPEXIT:.*]]:
+; CHECK-NEXT:    br label %[[FOR_COND_CLEANUP]]
+; CHECK:       [[FOR_COND_CLEANUP]]:
+; CHECK-NEXT:    ret void
+; CHECK:       [[FOR_BODY]]:
+; CHECK-NEXT:    [[INDVARS_IV:%.*]] = phi i64 [ [[INDVARS_IV_NEXT:%.*]], %[[FOR_INC:.*]] ], [ 0, %[[FOR_BODY_PREHEADER]] ]
+; CHECK-NEXT:    [[ARRAYIDX:%.*]] = getelementptr inbounds nuw i16, ptr [[X]], i64 [[INDVARS_IV]]
+; CHECK-NEXT:    [[TMP0:%.*]] = load i16, ptr [[ARRAYIDX]], align 2
+; CHECK-NEXT:    [[CONV:%.*]] = sext i16 [[TMP0]] to i32
+; CHECK-NEXT:    [[TMP1:%.*]] = sext i32 [[CONV]] to i64
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i64 [[INDVARS_IV]], [[TMP1]]
+; CHECK-NEXT:    br i1 [[CMP1]], label %[[IF_THEN:.*]], label %[[FOR_INC]]
+; CHECK:       [[IF_THEN]]:
+; CHECK-NEXT:    store i16 0, ptr [[ARRAYIDX]], align 2
+; CHECK-NEXT:    br label %[[FOR_INC]]
+; CHECK:       [[FOR_INC]]:
+; CHECK-NEXT:    [[INDVARS_IV_NEXT]] = add nuw nsw i64 [[INDVARS_IV]], 1
+; CHECK-NEXT:    [[EXITCOND:%.*]] = icmp ne i64 [[INDVARS_IV_NEXT]], [[WIDE_TRIP_COUNT]]
+; CHECK-NEXT:    br i1 [[EXITCOND]], label %[[FOR_BODY]], label %[[FOR_COND_CLEANUP_LOOPEXIT]]
+;
+entry:
+  %cmp10 = icmp sgt i32 %n, 0
+  br i1 %cmp10, label %for.body.preheader, label %for.cond.cleanup
+
+for.body.preheader:                               ; preds = %entry
+  br label %for.body
+
+for.cond.cleanup.loopexit:                        ; preds = %for.inc
+  br label %for.cond.cleanup
+
+for.cond.cleanup:                                 ; preds = %for.cond.cleanup.loopexit, %entry
+  ret void
+
+for.body:                                         ; preds = %for.body.preheader, %for.inc
+  %i.011 = phi i32 [ %inc, %for.inc ], [ 0, %for.body.preheader ]
+  %idxprom = zext nneg i32 %i.011 to i64
+  %arrayidx = getelementptr inbounds nuw i16, ptr %x, i64 %idxprom
+  %0 = load i16, ptr %arrayidx, align 2
+  %conv = sext i16 %0 to i32
+  %cmp1 = icmp eq i32 %i.011, %conv
+  br i1 %cmp1, label %if.then, label %for.inc
+
+if.then:                                          ; preds = %for.body
+  store i16 0, ptr %arrayidx, align 2
+  br label %for.inc
+
+for.inc:                                          ; preds = %for.body, %if.then
+  %inc = add nuw nsw i32 %i.011, 1
+  %cmp = icmp slt i32 %inc, %n
+  br i1 %cmp, label %for.body, label %for.cond.cleanup.loopexit
+}

dtcxzyw

As the narrow IV is always positive, can we use zext nneg instead?

topperc · 2025-06-04T16:06:13Z

As the narrow IV is always positive, can we use zext nneg instead?

We're extending the non-IV operand, we don't know about its sign bit.

preames · 2025-06-04T16:23:16Z

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp

@@ -1630,6 +1630,12 @@ bool WidenIV::widenLoopCompare(WidenIV::NarrowIVDefUse DU) {

  // Widen the other operand of the compare, if necessary.
  if (CastWidth < IVWidth) {
+    // If the narrow IV is always postive and the other operand is sext, widen


To check my understanding, this comment basically says that icmp PRED (zext nonneg X), (zext Y) == icmp PRED (zext nonneg X), (sext Y)?

Thinking through this...

If the high bit of X is set, then this is poison.

If the high bit of X is not set, then there are two cases:

The high bit of Y is not set, zext == sext by definition.

The high bit of Y is set, zext and sext produce different values. Neither can be equal to zext nonneg X (because it would be poison).

I can buy this for equality, but does this hold for inequality? In particular, icmp slt (zext nonneg X), ext(255) gives different results doesn't it?

Edit: Fixed a typo which changes meaning

I can buy this for equality, but does this hold for inequality? In particular, icmp slt (zext nonneg X), ext(255) gives different results doesn't it?

Was ext(255) supposed to be sext(255)

We were never going to use zext for a slt/sgt/sle/sge compare. That's checked earlier when CmpPreferredSign was first calculated. I've only changed what happens for eq/ne/ult/ugt/ule/uge.

Proof: https://alive2.llvm.org/ce/z/5vm_v5

The bit about not supporting the signed comparison was my key concern. I was going to reason through the unsigned variants, but the alive2 proof nicely does that. Can we add an assert here which checks the no-signed condition precondition?

@dtcxzyw - Your proof really makes me think we should have a instcombine transform which catches this form. Recognizing the icmp zext nonneg, zext (sext) form and replacing the two extends with one seems like a generally useful transform isn't it?

dtcxzyw

LG

dtcxzyw · 2025-06-09T03:16:54Z

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp

@@ -1630,6 +1630,12 @@ bool WidenIV::widenLoopCompare(WidenIV::NarrowIVDefUse DU) {

  // Widen the other operand of the compare, if necessary.
  if (CastWidth < IVWidth) {
+    // If the narrow IV is always postive and the other operand is sext, widen


Suggested change

// If the narrow IV is always postive and the other operand is sext, widen

// If the narrow IV is always non-negative and the other operand is sext, widen

I wrote "always postive" here because that's what is used 2 other times in the comment block earlier in this function. Should that be updated to use non-negative too?

dtcxzyw · 2025-06-09T03:17:34Z

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp

@@ -1630,6 +1630,12 @@ bool WidenIV::widenLoopCompare(WidenIV::NarrowIVDefUse DU) {

  // Widen the other operand of the compare, if necessary.
  if (CastWidth < IVWidth) {
+    // If the narrow IV is always postive and the other operand is sext, widen
+    // using sext so we can combine them. This works for all comparison


Suggested change

// using sext so we can combine them. This works for all comparison

// using sext so we can combine them. This works for all non-signed comparison

But it does work signed comparisons. Its just that signed comparisons were already going to use a sext unless samesign was set.

preames

LGTM

preames · 2025-06-09T15:16:29Z

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp

@@ -1630,6 +1630,12 @@ bool WidenIV::widenLoopCompare(WidenIV::NarrowIVDefUse DU) {

  // Widen the other operand of the compare, if necessary.
  if (CastWidth < IVWidth) {
+    // If the narrow IV is always postive and the other operand is sext, widen


The bit about not supporting the signed comparison was my key concern. I was going to reason through the unsigned variants, but the alive2 proof nicely does that. Can we add an assert here which checks the no-signed condition precondition?

@dtcxzyw - Your proof really makes me think we should have a instcombine transform which catches this form. Recognizing the icmp zext nonneg, zext (sext) form and replacing the two extends with one seems like a generally useful transform isn't it?

… and other operand is already sext. (llvm#142703) This prevents us from ending up with (zext (sext X)). The zext will require an instruction on targets where zext isn't free like RISC-V.

topperc added 2 commits June 3, 2025 16:52

Pre-commit test

da896b7

[IndVars] Teach widenLoopCompare to use sext if narrow IV is positive…

d0130a7

… and other operand is already sext. This prevents us from ending up with (zext (sext X)). The zext will require an instruction on targets where zext isn't free like RISC-V.

topperc requested review from nikic, preames and dtcxzyw June 4, 2025 00:25

llvmbot added the llvm:transforms label Jun 4, 2025

dtcxzyw reviewed Jun 4, 2025

View reviewed changes

preames reviewed Jun 4, 2025

View reviewed changes

dtcxzyw approved these changes Jun 9, 2025

View reviewed changes

preames approved these changes Jun 9, 2025

View reviewed changes

fixup! Address review comments

c853493

topperc merged commit e0cc556 into llvm:main Jun 10, 2025
5 of 7 checks passed

topperc deleted the pr/widenLoopCompar branch June 10, 2025 19:52

	// If the narrow IV is always postive and the other operand is sext, widen
	// If the narrow IV is always non-negative and the other operand is sext, widen

	// using sext so we can combine them. This works for all comparison
	// using sext so we can combine them. This works for all non-signed comparison

[IndVars] Teach widenLoopCompare to use sext if narrow IV is positive and other operand is already sext. #142703

[IndVars] Teach widenLoopCompare to use sext if narrow IV is positive and other operand is already sext. #142703

Uh oh!

Conversation

topperc commented Jun 4, 2025

Uh oh!

llvmbot commented Jun 4, 2025

Uh oh!

dtcxzyw left a comment

Choose a reason for hiding this comment

Uh oh!

topperc commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

preames Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dtcxzyw left a comment

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!

preames left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

topperc commented Jun 4, 2025 •

edited

Loading

preames Jun 4, 2025 •

edited

Loading