[Clang][CodeGen] Loose the cast check when emitting builtins

[shiltian]

This patch looses the cast check (canLosslesslyBitCastTo) and leaves it to the
one inside CreateBitCast. It seems too conservative for the use case here.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Shilei Tian (shiltian)

Changes

This patch looses the cast check (canLosslesslyBitCastTo) and leaves it to the
one inside CreateBitCast. It seems too conservative for the use case here.

---

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

1 Files Affected:

• (modified) clang/lib/CodeGen/CGBuiltin.cpp (-4)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index ee0b7504769622..9bc60466d09be6 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -5912,8 +5912,6 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
           }
         }
 
-        assert(ArgValue->getType()->canLosslesslyBitCastTo(PTy) &&
-               "Must be able to losslessly bit cast to param");
         // Cast vector type (e.g., v256i32) to x86_amx, this only happen
         // in amx intrinsics.
         if (PTy->isX86_AMXTy())
@@ -5943,8 +5941,6 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
         }
       }
 
-      assert(V->getType()->canLosslesslyBitCastTo(RetTy) &&
-             "Must be able to losslessly bit cast result type");
       // Cast x86_amx to vector type (e.g., v256i32), this only happen
       // in amx intrinsics.
       if (V->getType()->isX86_AMXTy())

[efriedma-quic]

I can't tell what you're trying to fix here. Is this fixing a crash? Or is the check redundant? Or is it necessary for some followup change you want to make?

[shiltian]

I can't tell what you're trying to fix here. Is this fixing a crash? Or is the check redundant? Or is it necessary for some followup change you want to make?

Sorry, I should have clearly mentioned that. Yes, it is for my followup change #80908. In #80908, we changed the type of LLVM builtin but kept the corresponding clang builtin unchanged to avoid breaking existing uses. Specifically, the original builtin accepts some bfloat arguments (either bfloat or <2 x bfloat>) and returns a bfloat value. Because of some historic reasons, the builtin uses i16 to represent bfloat values, and the backend just treats the 16 bits as bfloat. Now we encountered some issues that motivated us to use the right type here, but we still want to maintain sort of backward compatibility, at least on user-facing level. Keeping the corresponding clang builtin unchanged is the path we chose. Given that, it requires the front end to emit corresponding bitcast because the type i16 and bfloat (and the vec type) are not matched. EmitBuiltinExpr is capable of doing that, but the bitcast check here prevents it from doing it. canLosslesslyBitCastTo doesn't think bitcast from i16 to bf16 and vice versa are lossless. I was thinking of just allowing this specific cast in canLosslesslyBitCastTo but I figured that doesn't look very good. On a second thought, it looks like the check here is too conservative.

[arsenm]

Sorry, I should have clearly mentioned that. Yes, it is for my followup change #80908. In #80908, we changed the type of LLVM builtin but kept the corresponding clang builtin unchanged to avoid breaking existing uses.

Don't see how that could be related; you can losslessly bitconvert between i16 and bfloat

[Pierre-vh]

Sorry, I should have clearly mentioned that. Yes, it is for my followup change #80908. In #80908, we changed the type of LLVM builtin but kept the corresponding clang builtin unchanged to avoid breaking existing uses.

Don't see how that could be related; you can losslessly bitconvert between i16 and bfloat

I guess it's an oversight from that function to not allow FP <-> INT casts when they have the same width?
It seems like it'd let <4 x i16> to <4 x bf16> pass.

I think the right fix is teaching that function about FP <-> INT casts.
What do you think @nikic ?

[nikic]

I think the right fix is teaching that function about FP <-> INT casts.

The documentation for that function says:

  /// Return true if this type could be converted with a lossless BitCast to
  /// type 'Ty'. For example, i8* to i32*. BitCasts are valid for types of the
  /// same size only where no re-interpretation of the bits is done.

A cast between float and int sounds like "re-interpretation of the bits" to me. Though the function already allows it if it's a vector of int/float.

TBH I don't really understand what the purpose of this function is. One of the main uses seems to be to restrict what the returned attribute can be used for, but in that case we probably should stop accepting casts at all -- these don't really seem useful, and we've had crashes/miscompiles due to allowing them in the past.

For the purposes of this specific change, I think that just dropping the assert is indeed the right thing to do. The cast in the bitcast itself is sufficient.

[shiltian]

Don't see how that could be related; you can losslessly bitconvert between i16 and bfloat

Yes, canLosslesslyBitCastTo doesn't allow cast between i16 and bfloat, but it does between two vectors, as long as their sizes are the same.

A cast between float and int sounds like "re-interpretation of the bits" to me. Though the function already allows it if it's a vector of int/float.

Yeah. I was reluctant to allow it in canLosslesslyBitCastTo also because of that. Since it allows vector conversion, it already breaks the purpose.

[efriedma-quic]

LGTM

(It feels a little weird that the types in the clang AST don't actually match the LLVM builtin, but I guess it's not likely to actually cause any issues.)