[Clang][RISCV] Make generic clz/ctz builtins defined for zero on RISCV targets.

[Yunzezhu]

Summary

For now llvm intrinsic ctlz/cttz are supported by extension zbb and xtheadbb,
and both extensions support returning well-defined results for zero inputs.
It's possible to set isCLZForZeroUndef flag to false by default on RISCV targets
as ARM and other target does.

[craig.topper]

Doesn't this make codegen worse on when those extensions aren't supported?

[Yunzezhu]

I made the flag isCLZForZeroUndef set to false only when target support extension zbb or xtheadbb, and this will prevent making codegen worse when target does not support abb or xtheadbb.

[asb]

Could you please post a separate patch that has a test that will show the codegen change (and demonstrate how it is unchanged when zbb or xtheadbb)?

[Yunzezhu]

In D151867#4398483, @asb wrote:

Could you please post a separate patch that has a test that will show the codegen change (and demonstrate how it is unchanged when zbb or xtheadbb)?

Sure. I post a patch to demonstrate current codegen behavior for clz/ctz intrinsics when has ebb or xtheadbb: https://reviews.llvm.org/D152250.

[craig.topper]

From the C language perspective with this change, __builtin_clz/ctz is still considered undefined for 0 and code that uses it is ill-formed. isCLZForZeroUndef is only intended to prevent the middle end from optimizing based on the undefinedness and creating surprises. See also https://discourse.llvm.org/t/should-ubsan-detect-0-input-to-builtin-clz-ctz-regardless-of-target/71060

[Yunzezhu]

In D151867#4400255, @craig.topper wrote:

From the C language perspective with this change, __builtin_clz/ctz is still considered undefined for 0 and code that uses it is ill-formed. isCLZForZeroUndef is only intended to prevent the middle end from optimizing based on the undefinedness and creating surprises. See also https://discourse.llvm.org/t/should-ubsan-detect-0-input-to-builtin-clz-ctz-regardless-of-target/71060

I see builtin_clz/ctz returning an undefined value for 0 input matches gcc's document, but when I test builtin_clz/ctz with 0 input on gcc, it returns a valid value rather than an undefined value. It looks gcc does not follow gcc's document. I'm not sure which one is better that match document to return undefined for 0, or match gcc's behavior to return defined value?

[craig.topper]

In D151867#4401952, @Yunzezhu wrote:

In D151867#4400255, @craig.topper wrote:

From the C language perspective with this change, __builtin_clz/ctz is still considered undefined for 0 and code that uses it is ill-formed. isCLZForZeroUndef is only intended to prevent the middle end from optimizing based on the undefinedness and creating surprises. See also https://discourse.llvm.org/t/should-ubsan-detect-0-input-to-builtin-clz-ctz-regardless-of-target/71060

I see builtin_clz/ctz returning an undefined value for 0 input matches gcc's document, but when I test builtin_clz/ctz with 0 input on gcc, it returns a valid value rather than an undefined value. It looks gcc does not follow gcc's document. I'm not sure which one is better that match document to return undefined for 0, or match gcc's behavior to return defined value?

From what I can see in the assembly here https://godbolt.org/z/s4qqz83EK, gcc's undefined behavior sanitizer does consider an input of 0 to be undefined.

[Yunzezhu]

In D151867#4401987, @craig.topper wrote:

In D151867#4401952, @Yunzezhu wrote:

In D151867#4400255, @craig.topper wrote:

From the C language perspective with this change, __builtin_clz/ctz is still considered undefined for 0 and code that uses it is ill-formed. isCLZForZeroUndef is only intended to prevent the middle end from optimizing based on the undefinedness and creating surprises. See also https://discourse.llvm.org/t/should-ubsan-detect-0-input-to-builtin-clz-ctz-regardless-of-target/71060

I see builtin_clz/ctz returning an undefined value for 0 input matches gcc's document, but when I test builtin_clz/ctz with 0 input on gcc, it returns a valid value rather than an undefined value. It looks gcc does not follow gcc's document. I'm not sure which one is better that match document to return undefined for 0, or match gcc's behavior to return defined value?

From what I can see in the assembly here https://godbolt.org/z/s4qqz83EK, gcc's undefined behavior sanitizer does consider an input of 0 to be undefined.

Got it.

clang/lib/Basic/Targets/RISCV.h

public:
   }
 
   StringRef getABI() const override { return ABI; }
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
 
+  bool isCLZForZeroUndef() const override { return false; }
+
   BuiltinVaListKind getBuiltinVaListKind() const override {
     return TargetInfo::VoidPtrBuiltinVaList;
   }
 
   std::string_view getClobbers() const override { return ""; }
 
   StringRef getConstraintRegister(StringRef Constraint,
                                   StringRef Expression) const override {

clang/test/CodeGen/RISCV/rvb-intrinsics/riscv32-zbb.c

 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // RUN: %clang_cc1 -triple riscv32 -target-feature +zbb -emit-llvm %s -o - \
 // RUN:     | FileCheck %s  -check-prefix=RV32ZBB
 
 // RV32ZBB-LABEL: @orc_b_32(
 // RV32ZBB-NEXT:  entry:
 // RV32ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
 // RV32ZBB-NEXT:    store i32 [[A:%.*]], ptr [[A_ADDR]], align 4
 // RV32ZBB-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
 // RV32ZBB-NEXT:    [[TMP1:%.*]] = call i32 @llvm.riscv.orc.b.i32(i32 [[TMP0]])
 // RV32ZBB-NEXT:    ret i32 [[TMP1]]
 //
 int orc_b_32(int a) {
   return __builtin_riscv_orc_b_32(a);
 }
 
+// RV32ZBB-LABEL: @clz_32_generic(
+// RV32ZBB-NEXT:  entry:
+// RV32ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
+// RV32ZBB-NEXT:    store i32 [[A:%.*]], ptr [[A_ADDR]], align 4
+// RV32ZBB-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
+// RV32ZBB-NEXT:    [[TMP1:%.*]] = call i32 @llvm.ctlz.i32(i32 [[TMP0]], i1 false)
+// RV32ZBB-NEXT:    ret i32 [[TMP1]]
+//
+int clz_32_generic(int a) {
+  return __builtin_clz(a);
+}
+
 // RV32ZBB-LABEL: @clz_32(
 // RV32ZBB-NEXT:  entry:
 // RV32ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
 // RV32ZBB-NEXT:    store i32 [[A:%.*]], ptr [[A_ADDR]], align 4
 // RV32ZBB-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
 // RV32ZBB-NEXT:    [[TMP1:%.*]] = call i32 @llvm.ctlz.i32(i32 [[TMP0]], i1 false)
 // RV32ZBB-NEXT:    ret i32 [[TMP1]]
 //
 int clz_32(int a) {
   return __builtin_riscv_clz_32(a);
 }
 
+// RV32ZBB-LABEL: @ctz_32_generic(
+// RV32ZBB-NEXT:  entry:
+// RV32ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
+// RV32ZBB-NEXT:    store i32 [[A:%.*]], ptr [[A_ADDR]], align 4
+// RV32ZBB-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
+// RV32ZBB-NEXT:    [[TMP1:%.*]] = call i32 @llvm.cttz.i32(i32 [[TMP0]], i1 false)
+// RV32ZBB-NEXT:    ret i32 [[TMP1]]
+//
+int ctz_32_generic(int a) {
+  return __builtin_ctz(a);
+}
+
 // RV32ZBB-LABEL: @ctz_32(
 // RV32ZBB-NEXT:  entry:
 // RV32ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
 // RV32ZBB-NEXT:    store i32 [[A:%.*]], ptr [[A_ADDR]], align 4
 // RV32ZBB-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
 // RV32ZBB-NEXT:    [[TMP1:%.*]] = call i32 @llvm.cttz.i32(i32 [[TMP0]], i1 false)
 // RV32ZBB-NEXT:    ret i32 [[TMP1]]
 //
 int ctz_32(int a) {
   return __builtin_riscv_ctz_32(a);
 }
 No newline at end of file

clang/test/CodeGen/RISCV/rvb-intrinsics/riscv64-zbb.c

 // RV64ZBB-NEXT:    [[TMP0:%.*]] = load i64, ptr [[A_ADDR]], align 8
 // RV64ZBB-NEXT:    [[TMP1:%.*]] = call i64 @llvm.riscv.orc.b.i64(i64 [[TMP0]])
 // RV64ZBB-NEXT:    ret i64 [[TMP1]]
 //
 long orc_b_64(long a) {
   return __builtin_riscv_orc_b_64(a);
 }
 
+// RV64ZBB-LABEL: @clz_32_generic(
+// RV64ZBB-NEXT:  entry:
+// RV64ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
+// RV64ZBB-NEXT:    store i32 [[A:%.*]], ptr [[A_ADDR]], align 4
+// RV64ZBB-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
+// RV64ZBB-NEXT:    [[TMP1:%.*]] = call i32 @llvm.ctlz.i32(i32 [[TMP0]], i1 false)
+// RV64ZBB-NEXT:    ret i32 [[TMP1]]
+//
+int clz_32_generic(int a) {
+  return __builtin_clz(a);
+}
+
+// RV64ZBB-LABEL: @clz_64_generic(
+// RV64ZBB-NEXT:  entry:
+// RV64ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i64, align 8
+// RV64ZBB-NEXT:    store i64 [[A:%.*]], ptr [[A_ADDR]], align 8
+// RV64ZBB-NEXT:    [[TMP0:%.*]] = load i64, ptr [[A_ADDR]], align 8
+// RV64ZBB-NEXT:    [[TMP1:%.*]] = call i64 @llvm.ctlz.i64(i64 [[TMP0]], i1 false)
+// RV64ZBB-NEXT:    [[CAST:%.*]] = trunc i64 [[TMP1]] to i32
+// RV64ZBB-NEXT:    [[CONV:%.*]] = sext i32 [[CAST]] to i64
+// RV64ZBB-NEXT:    ret i64 [[CONV]]
+//
+long clz_64_generic(long a) {
+  return __builtin_clzl(a);
+}
+
 // RV64ZBB-LABEL: @clz_32(
 // RV64ZBB-NEXT:  entry:
 // RV64ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
 // RV64ZBB-NEXT:    store i32 [[A:%.*]], ptr [[A_ADDR]], align 4
 // RV64ZBB-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
 // RV64ZBB-NEXT:    [[TMP1:%.*]] = call i32 @llvm.ctlz.i32(i32 [[TMP0]], i1 false)
 // RV64ZBB-NEXT:    ret i32 [[TMP1]]
 //
 int clz_32(int a) {
   return __builtin_riscv_clz_32(a);
 }
 
 // RV64ZBB-LABEL: @clz_64(
 // RV64ZBB-NEXT:  entry:
 // RV64ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i64, align 8
 // RV64ZBB-NEXT:    store i64 [[A:%.*]], ptr [[A_ADDR]], align 8
 // RV64ZBB-NEXT:    [[TMP0:%.*]] = load i64, ptr [[A_ADDR]], align 8
 // RV64ZBB-NEXT:    [[TMP1:%.*]] = call i64 @llvm.ctlz.i64(i64 [[TMP0]], i1 false)
 // RV64ZBB-NEXT:    ret i64 [[TMP1]]
 //
 long clz_64(long a) {
   return __builtin_riscv_clz_64(a);
 }
 
+// RV64ZBB-LABEL: @ctz_32_generic(
+// RV64ZBB-NEXT:  entry:
+// RV64ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
+// RV64ZBB-NEXT:    store i32 [[A:%.*]], ptr [[A_ADDR]], align 4
+// RV64ZBB-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
+// RV64ZBB-NEXT:    [[TMP1:%.*]] = call i32 @llvm.cttz.i32(i32 [[TMP0]], i1 false)
+// RV64ZBB-NEXT:    ret i32 [[TMP1]]
+//
+int ctz_32_generic(int a) {
+  return __builtin_ctz(a);
+}
+
+// RV64ZBB-LABEL: @ctz_64_generic(
+// RV64ZBB-NEXT:  entry:
+// RV64ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i64, align 8
+// RV64ZBB-NEXT:    store i64 [[A:%.*]], ptr [[A_ADDR]], align 8
+// RV64ZBB-NEXT:    [[TMP0:%.*]] = load i64, ptr [[A_ADDR]], align 8
+// RV64ZBB-NEXT:    [[TMP1:%.*]] = call i64 @llvm.cttz.i64(i64 [[TMP0]], i1 false)
+// RV64ZBB-NEXT:    [[CAST:%.*]] = trunc i64 [[TMP1]] to i32
+// RV64ZBB-NEXT:    [[CONV:%.*]] = sext i32 [[CAST]] to i64
+// RV64ZBB-NEXT:    ret i64 [[CONV]]
+//
+long ctz_64_generic(long a) {
+  return __builtin_ctzl(a);
+}
+
 // RV64ZBB-LABEL: @ctz_32(
 // RV64ZBB-NEXT:  entry:
 // RV64ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i32, align 4
 // RV64ZBB-NEXT:    store i32 [[A:%.*]], ptr [[A_ADDR]], align 4
 // RV64ZBB-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A_ADDR]], align 4
 // RV64ZBB-NEXT:    [[TMP1:%.*]] = call i32 @llvm.cttz.i32(i32 [[TMP0]], i1 false)
 // RV64ZBB-NEXT:    ret i32 [[TMP1]]
 //
 int ctz_32(int a) {
   return __builtin_riscv_ctz_32(a);
 }
 
 // RV64ZBB-LABEL: @ctz_64(
 // RV64ZBB-NEXT:  entry:
 // RV64ZBB-NEXT:    [[A_ADDR:%.*]] = alloca i64, align 8
 // RV64ZBB-NEXT:    store i64 [[A:%.*]], ptr [[A_ADDR]], align 8
 // RV64ZBB-NEXT:    [[TMP0:%.*]] = load i64, ptr [[A_ADDR]], align 8
 // RV64ZBB-NEXT:    [[TMP1:%.*]] = call i64 @llvm.cttz.i64(i64 [[TMP0]], i1 false)
 // RV64ZBB-NEXT:    ret i64 [[TMP1]]
 //
 long ctz_64(long a) {
   return __builtin_riscv_ctz_64(a);
 }
 No newline at end of file