[PATCH] D12313: Introduce __builtin_nontemporal_store and __builtin_nontemporal_load.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Basic/Builtins.def:1249-1255
@@ +1248,9 @@
+BUILTIN(__builtin_nontemporal_store, "v.", "t")
+BUILTIN(__builtin_nontemporal_store_1, "vcc*.", "")
+BUILTIN(__builtin_nontemporal_store_2, "vss*.", "")
+BUILTIN(__builtin_nontemporal_store_4, "vii*.", "")
+BUILTIN(__builtin_nontemporal_store_8, "vLLiLLi*.", "")
+BUILTIN(__builtin_nontemporal_store_16, "vLLLiLLLi*.", "")
+BUILTIN(__builtin_nontemporal_store_f, "vff*.", "")
+BUILTIN(__builtin_nontemporal_store_d, "vdd*.", "")
+
----------------
Do we need these typed versions?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6198-6203
@@ +6197,8 @@
+  "address argument to nontemporal builtin must be a pointer (%0 invalid)">;
+def err_nontemporal_builtin_must_be_pointer_intfltptr : Error<
+  "address argument to nontemporal builtin must be a pointer to integer, float "
+  "or pointer (%0 invalid)">;
+def err_nontemporal_builtin_pointer_size : Error<
+  "address argument to nontemporal builtin must be a pointer to 1,2,4,8 or 16 "
+  "byte type (%0 invalid)">;
+
----------------
Nontemporal loads and stores of, say, vector types seem useful, but I can understand if you want to restrict the complexity of the CodeGen changes here.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:128-129
@@ +127,4 @@
+  // Convert the type of the pointer to a pointer to the stored type.
+  Value *BC = CGF.Builder.CreateBitCast(
+      Address, llvm::PointerType::getUnqual(ValueTy), "cast");
+  StoreInst *SI = CGF.Builder.CreateStore(Val, BC);
----------------
You should pass `Val` through `EmitToMemory` first to widen `bool` from `i1` to `i8`.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:149
@@ +148,3 @@
+  LI->setAlignment(Align);
+  return LI;
+}
----------------
Likewise, you should pass this though `EmitFromMemory`.

================
Comment at: lib/Sema/SemaChecking.cpp:2235-2241
@@ +2234,9 @@
+
+  // Ensure that we have at least one argument to do type inference from.
+  if (TheCall->getNumArgs() < numArgs) {
+    Diag(TheCall->getLocEnd(), diag::err_typecheck_call_too_few_args_at_least)
+        << 0 << 1 << TheCall->getNumArgs()
+        << TheCall->getCallee()->getSourceRange();
+    return ExprError();
+  }
+
----------------
You should also reject calls with too many arguments here. Maybe just call `checkArgCount(*this, TheCall, numArgs)`?

================
Comment at: lib/Sema/SemaChecking.cpp:2275-2357
@@ +2274,85 @@
+
+#define BUILTIN_ROW(x)                                                         \
+  { Builtin::BI##x##_1, Builtin::BI##x##_2, Builtin::BI##x##_4,                \
+    Builtin::BI##x##_8, Builtin::BI##x##_16,                                   \
+    Builtin::BI##x##_f, Builtin::BI##x##_d }
+
+  static const unsigned BuiltinIndices[][7] = {
+      BUILTIN_ROW(__builtin_nontemporal_store),
+      BUILTIN_ROW(__builtin_nontemporal_load)
+  };
+#undef BUILTIN_ROW
+
+  // Determine the index of the size.
+  unsigned SizeIndex;
+  if (ValType->isFloatingType()) {
+    switch (Context.getTypeSizeInChars(ValType).getQuantity()) {
+    case 4: SizeIndex = 5; break;
+    case 8: SizeIndex = 6; break;
+    default:
+      Diag(DRE->getLocStart(), diag::err_nontemporal_builtin_pointer_size)
+          << AddressArg->getType() << AddressArg->getSourceRange();
+      return ExprError();
+    }
+  } else {
+    switch (Context.getTypeSizeInChars(ValType).getQuantity()) {
+    case 1: SizeIndex = 0; break;
+    case 2: SizeIndex = 1; break;
+    case 4: SizeIndex = 2; break;
+    case 8: SizeIndex = 3; break;
+    case 16: SizeIndex = 4; break;
+    default:
+      Diag(DRE->getLocStart(), diag::err_nontemporal_builtin_pointer_size)
+          << AddressArg->getType() << AddressArg->getSourceRange();
+      return ExprError();
+    }
+  }
+  // Get the decl for the concrete builtin from this, we can tell what the
+  // concrete integer type we should convert to is.
+  unsigned NewBuiltinID = BuiltinIndices[isStore ? 0 : 1][SizeIndex];
+  const char *NewBuiltinName = Context.BuiltinInfo.getName(NewBuiltinID);
+
+  // Perform builtin lookup to avoid redeclaring it.
+  DeclarationName DN(&Context.Idents.get(NewBuiltinName));
+  LookupResult Res(*this, DN, DRE->getLocStart(), LookupOrdinaryName);
+  LookupName(Res, TUScope, /*AllowBuiltinCreation=*/true);
+  assert(Res.getFoundDecl());
+  FunctionDecl *NewBuiltinDecl = dyn_cast<FunctionDecl>(Res.getFoundDecl());
+  if (!NewBuiltinDecl)
+    return ExprError();
+
+  // For stores convert the value (the first argument) to the type of the
+  // pointer (the second argument).
+  if (isStore) {
+    ExprResult Arg = TheCall->getArg(0);
+    InitializedEntity Entity = InitializedEntity::InitializeParameter(
+        Context, ValType, /*consume*/ false);
+    Arg = PerformCopyInitialization(Entity, SourceLocation(), Arg);
+    if (Arg.isInvalid())
+      return ExprError();
+
+    TheCall->setArg(0, Arg.get());
+  }
+
+  ASTContext &Context = this->getASTContext();
+
+  // Create a new DeclRefExpr to refer to the new decl.
+  DeclRefExpr *NewDRE = DeclRefExpr::Create(
+      Context, DRE->getQualifierLoc(), SourceLocation(), NewBuiltinDecl,
+      /*enclosing*/ false, DRE->getLocation(), Context.BuiltinFnTy,
+      DRE->getValueKind());
+
+  // Set the callee in the CallExpr.
+  // FIXME: This loses syntactic information.
+  QualType CalleePtrTy = Context.getPointerType(NewBuiltinDecl->getType());
+  ExprResult PromotedCall =
+      ImpCastExprToType(NewDRE, CalleePtrTy, CK_BuiltinFnToFnPtr);
+  TheCall->setCallee(PromotedCall.get());
+
+  // For loads change the result type of the call to match the original value
+  // type. This is arbitrary, but the codegen for these builtins ins design to
+  // handle it gracefully.
+  if (!isStore)
+    TheCall->setType(ValType);
+
+  return TheCallResult;
----------------
Instead of doing all of this, you can just (for stores) check the type of the second parameter or (for loads) set the type of the call expression. (See `CheckARMBuiltinExclusiveCall` for an example.)


http://reviews.llvm.org/D12313