fix: review

Author: liuzhenya

clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h

@@ -137,7 +137,7 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
     if (auto floatType = mlir::dyn_cast<cir::FPTypeInterface>(eltTy))
       return floatType.getWidth();
 
-    llvm_unreachable("Wrong type passed in or Non-CIR type passed in");
+    llvm_unreachable("Unsupported type in getCIRIntOrFloatBitWidth");
   }
   cir::IntType getSIntNTy(int n) {
     return cir::IntType::get(getContext(), n, true);

clang/lib/CIR/CodeGen/CIRGenBuilder.h

@@ -27,26 +27,13 @@ namespace clang::CIRGen {
 
 class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
   const CIRGenTypeCache &typeCache;
-  bool isFPConstrained = false;
   llvm::StringMap<unsigned> recordNames;
   llvm::StringMap<unsigned> globalsVersioning;
 
 public:
   CIRGenBuilderTy(mlir::MLIRContext &mlirContext, const CIRGenTypeCache &tc)
       : CIRBaseBuilderTy(mlirContext), typeCache(tc) {}
 
-  //
-  // Floating point specific helpers
-  // -------------------------------
-  //
-
-  /// Query for the use of constrained floating point math
-  bool getisFPConstrained() {
-    if (isFPConstrained)
-      llvm_unreachable("Constrained FP NYI");
-    return isFPConstrained;
-  }
-
   /// Get a cir::ConstArrayAttr for a string literal.
   /// Note: This is different from what is returned by
   /// mlir::Builder::getStringAttr() which is an mlir::StringAttr.

clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp

@@ -631,17 +631,17 @@ CIRGenFunction::emitTargetBuiltinExpr(unsigned builtinID, const CallExpr *e,
 // a ConstantInt. Otherwise we emit it as a normal scalar value.
 mlir::Value CIRGenFunction::emitScalarOrConstFoldImmArg(unsigned iceArguments,
                                                         unsigned idx,
-                                                        const CallExpr *expr) {
+                                                        const Expr *argExpr) {
   mlir::Value arg = {};
   if ((iceArguments & (1 << idx)) == 0) {
-    arg = emitScalarExpr(expr->getArg(idx));
+    arg = emitScalarExpr(argExpr);
   } else {
     // If this is required to be a constant, constant fold it so that we
     // know that the generated intrinsic gets a ConstantInt.
     std::optional<llvm::APSInt> result =
-        expr->getArg(idx)->getIntegerConstantExpr(getContext());
+        argExpr->getIntegerConstantExpr(getContext());
     assert(result && "Expected argument to be a constant");
-    arg = builder.getConstInt(getLoc(expr->getSourceRange()), *result);
+    arg = builder.getConstInt(getLoc(argExpr->getSourceRange()), *result);
   }
   return arg;
 }

clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp

@@ -21,6 +21,41 @@
 using namespace clang;
 using namespace clang::CIRGen;
 
+// OG has unordered comparison as a form of optimization in addition to
+// ordered comparison, while CIR doesn't.
+//
+// This means that we can't encode the comparison code of UGT (unordered
+// greater than), at least not at the CIR level.
+//
+// The boolean shouldInvert compensates for this.
+// For example: to get to the comparison code UGT, we pass in
+// emitVectorFCmp (OLE, shouldInvert = true) since OLE is the inverse of UGT.
+
+// There are several ways to support this otherwise:
+// - register extra CmpOpKind for unordered comparison types and build the
+// translation code for
+//    to go from CIR -> LLVM dialect. Notice we get this naturally with
+//    shouldInvert, benefiting from existing infrastructure, albeit having to
+//    generate an extra `not` at CIR).
+// - Just add extra comparison code to a new VecCmpOpKind instead of
+// cluttering CmpOpKind.
+// - Add a boolean in VecCmpOp to indicate if it's doing unordered or ordered
+// comparison
+// - Just emit the intrinsics call instead of calling this helper, see how the
+// LLVM lowering handles this.
+static mlir::Value emitVectorFCmp(CIRGenBuilderTy &builder,
+                                  llvm::SmallVector<mlir::Value> &ops,
+                                  mlir::Location loc, cir::CmpOpKind pred,
+                                  bool shouldInvert) {
+  assert(!cir::MissingFeatures::cgFPOptionsRAII());
+  // TODO(cir): Add isSignaling boolean once emitConstrainedFPCall implemented
+  assert(!cir::MissingFeatures::emitConstrainedFPCall());
+  mlir::Value cmp = builder.createVecCompare(loc, pred, ops[0], ops[1]);
+  mlir::Value bitCast = builder.createBitcast(
+      shouldInvert ? builder.createNot(cmp) : cmp, ops[0].getType());
+  return bitCast;
+}
+
 mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned builtinID,
                                                const CallExpr *expr) {
   if (builtinID == Builtin::BI__builtin_cpu_is) {
@@ -52,45 +87,9 @@ mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned builtinID,
   assert(error == ASTContext::GE_None &&
          "builtinID should be checked before calling emitX86BuiltinExpr");
   for (auto [idx, arg] : llvm::enumerate(expr->arguments())) {
-    ops.push_back(emitScalarOrConstFoldImmArg(iceArguments, idx, expr));
+    ops.push_back(emitScalarOrConstFoldImmArg(iceArguments, idx, arg));
   }
 
-  // OG has unordered comparison as a form of optimization in addition to
-  // ordered comparison, while CIR doesn't.
-  //
-  // This means that we can't encode the comparison code of UGT (unordered
-  // greater than), at least not at the CIR level.
-  //
-  // The boolean shouldInvert compensates for this.
-  // For example: to get to the comparison code UGT, we pass in
-  // emitVectorFCmp (OLE, shouldInvert = true) since OLE is the inverse of UGT.
-
-  // There are several ways to support this otherwise:
-  // - register extra CmpOpKind for unordered comparison types and build the
-  // translation code for
-  //    to go from CIR -> LLVM dialect. Notice we get this naturally with
-  //    shouldInvert, benefiting from existing infrastructure, albeit having to
-  //    generate an extra `not` at CIR).
-  // - Just add extra comparison code to a new VecCmpOpKind instead of
-  // cluttering CmpOpKind.
-  // - Add a boolean in VecCmpOp to indicate if it's doing unordered or ordered
-  // comparison
-  // - Just emit the intrinsics call instead of calling this helper, see how the
-  // LLVM lowering handles this.
-  auto emitVectorFCmp = [this, &ops, &expr](cir::CmpOpKind pred,
-                                            bool shouldInvert,
-                                            bool isSignaling) {
-    assert(!cir::MissingFeatures::cgFPOptionsRAII());
-    auto loc = getLoc(expr->getExprLoc());
-    // TODO: Add isSignaling boolean once emitConstrainedFPCall implemented
-    assert(!cir::MissingFeatures::emitConstrainedFPCall() &&
-           "constrained FP compare NYI");
-    mlir::Value cmp = builder.createVecCompare(loc, pred, ops[0], ops[1]);
-    mlir::Value bitCast = builder.createBitcast(
-        shouldInvert ? builder.createNot(cmp) : cmp, ops[0].getType());
-    return bitCast;
-  };
-
   switch (builtinID) {
   default:
     return {};
@@ -764,12 +763,12 @@ mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned builtinID,
     return {};
   case X86::BI__builtin_ia32_cmpnltps:
   case X86::BI__builtin_ia32_cmpnltpd:
-    return emitVectorFCmp(cir::CmpOpKind::lt, /*shouldInvert=*/true,
-                          /*isSignaling=*/true);
+    return emitVectorFCmp(builder, ops, getLoc(expr->getExprLoc()),
+                          cir::CmpOpKind::lt, /*shouldInvert=*/true);
   case X86::BI__builtin_ia32_cmpnleps:
   case X86::BI__builtin_ia32_cmpnlepd:
-    return emitVectorFCmp(cir::CmpOpKind::le, /*shouldInvert=*/true,
-                          /*isSignaling=*/true);
+    return emitVectorFCmp(builder, ops, getLoc(expr->getExprLoc()),
+                          cir::CmpOpKind::le, /*shouldInvert=*/true);
   case X86::BI__builtin_ia32_cmpordps:
   case X86::BI__builtin_ia32_cmpordpd:
   case X86::BI__builtin_ia32_cmpph128_mask:

clang/lib/CIR/CodeGen/CIRGenFunction.h

@@ -1700,7 +1700,7 @@ class CIRGenFunction : public CIRGenTypeCache {
                       LValue lvalue, bool capturedByInit = false);
 
   mlir::Value emitScalarOrConstFoldImmArg(unsigned iceArguments, unsigned idx,
-                                          const CallExpr *expr);
+                                          const Expr *argExpr);
 
   void emitStaticVarDecl(const VarDecl &d, cir::GlobalLinkageKind linkage);