Fix Issue #56706 (maskl(0), maskr(0) return -1 instead of 0)

[tarunprabhu]

Summary

Fix 56706.

Add an explicit condition for the BITS = 0 case.

[clementval]

Add an explicit condition for the BITS = 0 case

This is probably a better commit title. I would put your title as description.

[jeanPerier]

I agree with clementval's comment about the commit title (should also have [flang]), otherwise LGTM, thanks for fixing the bug !

[clementval]

Is there any reason you landed the patch without acceptance?

[tarunprabhu]

Oh, I'm sorry. I should have asked for acceptance before committing.

There were no comments on this for some time and the other comments suggested that it was fine. I made the change to flip the title and the commit message in the actual commit.

My apologies again. Should I revert that commit?

[clementval]

Looks ok. You don't need to revert. Next time wait for a formal acceptance or a "LGTM". No comment does not mean approved. Personally I did not go through the whole patch when I sent the previous comment.

[tarunprabhu]

Noted. In my defense, there was a LGTM from @jeanPerier.

In D130590#3682021, @jeanPerier wrote:

I agree with clementval's comment about the commit title (should also have [flang]), otherwise LGTM, thanks for fixing the bug !

But I'll wait for a formal acceptance so there's no confusion.

[clementval]

In D130590#3707536, @tarunprabhu wrote:

Noted. In my defense, there was a LGTM from @jeanPerier.

In D130590#3682021, @jeanPerier wrote:

I agree with clementval's comment about the commit title (should also have [flang]), otherwise LGTM, thanks for fixing the bug !

But I'll wait for a formal acceptance so there's no confusion.

My bad I did not notice it.

[tarunprabhu]

All's well that ends well ... :-)

flang/lib/Lower/IntrinsicCall.cpp

 }
 
 // MASKL, MASKR
 template <typename Shift>
 mlir::Value IntrinsicLibrary::genMask(mlir::Type resultType,
                                       llvm::ArrayRef<mlir::Value> args) {
   assert(args.size() == 2);
 
+  mlir::Value zero = builder.createIntegerConstant(loc, resultType, 0);
   mlir::Value ones = builder.createIntegerConstant(loc, resultType, -1);
   mlir::Value bitSize = builder.createIntegerConstant(
       loc, resultType, resultType.getIntOrFloatBitWidth());
   mlir::Value bitsToSet = builder.createConvert(loc, resultType, args[0]);
 
   // The standard does not specify what to return if the number of bits to be
   // set, I < 0 or I >= BIT_SIZE(KIND). The shift instruction used below will
   // produce a poison value which may return a possibly platform-specific and/or
   // non-deterministic result. Other compilers don't produce a consistent result
   // in this case either, so we choose the most efficient implementation.
   mlir::Value shift =
       builder.create<mlir::arith::SubIOp>(loc, bitSize, bitsToSet);
-  return builder.create<Shift>(loc, ones, shift);
+  mlir::Value shifted = builder.create<Shift>(loc, ones, shift);
+  mlir::Value isZero = builder.create<mlir::arith::CmpIOp>(
+      loc, mlir::arith::CmpIPredicate::eq, bitsToSet, zero);
+
+  return builder.create<mlir::arith::SelectOp>(loc, isZero, zero, shifted);
 }
 
 // MATMUL
 fir::ExtendedValue
 IntrinsicLibrary::genMatmul(mlir::Type resultType,
                             llvm::ArrayRef<fir::ExtendedValue> args) {
   assert(args.size() == 2);
 

flang/test/Lower/Intrinsics/maskl.f90

 ! RUN: bbc -emit-fir %s -o - | FileCheck %s
 ! RUN: %flang_fc1 -emit-fir %s -o - | FileCheck %s
 
 ! CHECK-LABEL: maskl_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i32>{{.*}}
 subroutine maskl_test(a, b)
   integer :: a
   integer :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskl(a)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i32
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i32
   ! CHECK: %[[BITS:.*]] = arith.constant 32 : i32
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_VAL]] : i32
   ! CHECK: %[[SHIFT:.*]] = arith.shli %[[C__1]], %[[LEN]] : i32
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i32>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_VAL]], %[[C__0]] : i32
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i32
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i32>
 end subroutine maskl_test
 
 ! CHECK-LABEL: maskl1_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i8>{{.*}}
 subroutine maskl1_test(a, b)
   integer :: a
   integer(kind=1) :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskl(a, 1)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i8
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i8
   ! CHECK: %[[BITS:.*]] = arith.constant 8 : i8
   ! CHECK: %[[A_CONV:.*]] = fir.convert %[[A_VAL]] : (i32) -> i8
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_CONV]] : i8
   ! CHECK: %[[SHIFT:.*]] = arith.shli %[[C__1]], %[[LEN]] : i8
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i8>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_CONV]], %[[C__0]] : i8
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i8
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i8>
 end subroutine maskl1_test
 
 ! CHECK-LABEL: maskl2_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i16>{{.*}}
 subroutine maskl2_test(a, b)
   integer :: a
   integer(kind=2) :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskl(a, 2)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i16
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i16
   ! CHECK: %[[BITS:.*]] = arith.constant 16 : i16
   ! CHECK: %[[A_CONV:.*]] = fir.convert %[[A_VAL]] : (i32) -> i16
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_CONV]] : i16
   ! CHECK: %[[SHIFT:.*]] = arith.shli %[[C__1]], %[[LEN]] : i16
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i16>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_CONV]], %[[C__0]] : i16
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i16
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i16>
 end subroutine maskl2_test
 
 ! CHECK-LABEL: maskl4_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i32>{{.*}}
 subroutine maskl4_test(a, b)
   integer :: a
   integer(kind=4) :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskl(a, 4)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i32
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i32
   ! CHECK: %[[BITS:.*]] = arith.constant 32 : i32
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_VAL]] : i32
   ! CHECK: %[[SHIFT:.*]] = arith.shli %[[C__1]], %[[LEN]] : i32
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i32>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_VAL]], %[[C__0]] : i32
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i32
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i32>
 end subroutine maskl4_test
 
 ! CHECK-LABEL: maskl8_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i64>{{.*}}
 subroutine maskl8_test(a, b)
   integer :: a
   integer(kind=8) :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskl(a, 8)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i64
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i64
   ! CHECK: %[[BITS:.*]] = arith.constant 64 : i64
   ! CHECK: %[[A_CONV:.*]] = fir.convert %[[A_VAL]] : (i32) -> i64
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_CONV]] : i64
   ! CHECK: %[[SHIFT:.*]] = arith.shli %[[C__1]], %[[LEN]] : i64
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i64>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_CONV]], %[[C__0]] : i64
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i64
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i64>
 end subroutine maskl8_test
 
 ! TODO: Code containing 128-bit integer literals current breaks. This is
 ! probably related to the issue linked below. When that is fixed, a test
 ! for kind=16 should be added here.
 !
 ! https://github.com/llvm/llvm-project/issues/56446

flang/test/Lower/Intrinsics/maskr.f90

 ! RUN: bbc -emit-fir %s -o - | FileCheck %s
 ! RUN: %flang_fc1 -emit-fir %s -o - | FileCheck %s
 
 ! CHECK-LABEL: maskr_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i32>{{.*}}
 subroutine maskr_test(a, b)
   integer :: a
   integer :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskr(a)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i32
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i32
   ! CHECK: %[[BITS:.*]] = arith.constant 32 : i32
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_VAL]] : i32
   ! CHECK: %[[SHIFT:.*]] = arith.shrui %[[C__1]], %[[LEN]] : i32
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i32>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_VAL]], %[[C__0]] : i32
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i32
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i32>
 end subroutine maskr_test
 
 ! CHECK-LABEL: maskr1_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i8>{{.*}}
 subroutine maskr1_test(a, b)
   integer :: a
   integer(kind=1) :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskr(a, 1)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i8
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i8
   ! CHECK: %[[BITS:.*]] = arith.constant 8 : i8
   ! CHECK: %[[A_CONV:.*]] = fir.convert %[[A_VAL]] : (i32) -> i8
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_CONV]] : i8
   ! CHECK: %[[SHIFT:.*]] = arith.shrui %[[C__1]], %[[LEN]] : i8
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i8>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_CONV]], %[[C__0]] : i8
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i8
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i8>
 end subroutine maskr1_test
 
 ! CHECK-LABEL: maskr2_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i16>{{.*}}
 subroutine maskr2_test(a, b)
   integer :: a
   integer(kind=2) :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskr(a, 2)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i16
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i16
   ! CHECK: %[[BITS:.*]] = arith.constant 16 : i16
   ! CHECK: %[[A_CONV:.*]] = fir.convert %[[A_VAL]] : (i32) -> i16
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_CONV]] : i16
   ! CHECK: %[[SHIFT:.*]] = arith.shrui %[[C__1]], %[[LEN]] : i16
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i16>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_CONV]], %[[C__0]] : i16
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i16
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i16>
 end subroutine maskr2_test
 
 ! CHECK-LABEL: maskr4_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i32>{{.*}}
 subroutine maskr4_test(a, b)
   integer :: a
   integer(kind=4) :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskr(a, 4)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i32
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i32
   ! CHECK: %[[BITS:.*]] = arith.constant 32 : i32
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_VAL]] : i32
   ! CHECK: %[[SHIFT:.*]] = arith.shrui %[[C__1]], %[[LEN]] : i32
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i32>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_VAL]], %[[C__0]] : i32
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i32
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i32>
 end subroutine maskr4_test
 
 ! CHECK-LABEL: maskr8_test
 ! CHECK-SAME: %[[A:.*]]: !fir.ref<i32>{{.*}}, %[[B:.*]]: !fir.ref<i64>{{.*}}
 subroutine maskr8_test(a, b)
   integer :: a
   integer(kind=8) :: b
 
   ! CHECK: %[[A_VAL:.*]] = fir.load %[[A]] : !fir.ref<i32>
   b = maskr(a, 8)
+  ! CHECK: %[[C__0:.*]] = arith.constant 0 : i64
   ! CHECK: %[[C__1:.*]] = arith.constant -1 : i64
   ! CHECK: %[[BITS:.*]] = arith.constant 64 : i64
   ! CHECK: %[[A_CONV:.*]] = fir.convert %[[A_VAL]] : (i32) -> i64
   ! CHECK: %[[LEN:.*]] = arith.subi %[[BITS]], %[[A_CONV]] : i64
   ! CHECK: %[[SHIFT:.*]] = arith.shrui %[[C__1]], %[[LEN]] : i64
-  ! CHECK: fir.store %[[SHIFT]] to %[[B]] : !fir.ref<i64>
+  ! CHECK: %[[IS0:.*]] = arith.cmpi eq, %[[A_CONV]], %[[C__0]] : i64
+  ! CHECK: %[[RESULT:.*]] = arith.select %[[IS0]], %[[C__0]], %[[SHIFT]] : i64
+  ! CHECK: fir.store %[[RESULT]] to %[[B]] : !fir.ref<i64>
 end subroutine maskr8_test
 
 ! TODO: Code containing 128-bit integer literals current breaks. This is
 ! probably related to the issue linked below. When that is fixed, a test
 ! for kind=16 should be added here.
 !
 ! https://github.com/llvm/llvm-project/issues/56446