[IR][RFC] Restrict read only when cache type of llvm.prefetch is instruction

[pengfei]

Summary

LLVM backends like X86 always lowers prefetch intrinsics into similar
type if the desired one is not available.
For example, T2 hint -> T1 or write hint -> read. See llvm/test/CodeGen/X86/prefetch.ll
However, it's not clear for backend to choose between "write data" or "read
instruction" if there is no native "write instruction" instructions.
As far as I know, there's no upstream target that has supported "write
instruction" at the moment. So there should be no real impact by this
patch.

[lenary]

The Arm changes (for tests) here are reasonable, and indeed both arm architectures do not allocate encoding space for instruction write.

[pengfei]

In D136145#3864584, @lenary wrote:

The Arm changes (for tests) here are reasonable, and indeed both arm architectures do not allocate encoding space for instruction write.

Thanks @lenary for confirmation!

[uweigand]

llvm/test/CodeGen/SystemZ/prefetch-01.ll

17–18

If we decide to declare this invalid, then I'd prefer to remove the test instead of commenting it out.

[pengfei]

Remove instruction write prefetch test from SystemZ.

[craig.topper]

clang/lib/Sema/SemaChecking.cpp

7574

Not clear to me that we should be changing the definition of __builtin_prefetch.

It wouldn't cost much to add a new builtin for X86 for the new instructions.

[craig.topper]

clang/lib/Sema/SemaChecking.cpp

7574

It definitely shouldn't be buried in this patch the way it is currently titled and described.

[pengfei]

clang/lib/Sema/SemaChecking.cpp

7574

Sure, I have added a new builtin in D136040. But I don't see any concerns for this patch so far.

[arsenm]

llvm/lib/IR/Verifier.cpp

5180

Should add new verifier tests for this.

Also, if this requires any auto upgrade, I would appreciate adding address space mangling to the intrinsic at the same time

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_typecheck_call_too_many_args_suggest : Error<
   "too many %select{|||execution configuration }0arguments to "
   "%select{function|block|method|kernel function}0 call, "
   "expected %1, have %2; did you mean %3?">;
 def err_typecheck_call_too_many_args_at_most_suggest : Error<
   "too many %select{|||execution configuration }0arguments to "
   "%select{function|block|method|kernel function}0 call, "
   "expected at most %1, have %2; did you mean %3?">;
+def err_typecheck_call_inst_cache_must_read_only : Error<
+  "instruction cache must be read only">;
 
 def err_arc_typecheck_convert_incompatible_pointer : Error<
   "incompatible pointer types passing retainable parameter of type %0"
   "to a CF function expecting %1 type">;
 
 def err_builtin_fn_use : Error<"builtin functions must be directly called">;
 
 def warn_call_wrong_number_of_arguments : Warning<

clang/lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_object_size: {
 
     // We pass this builtin onto the optimizer so that it can figure out the
     // object size in more complex cases.
     bool IsDynamic = BuiltinID == Builtin::BI__builtin_dynamic_object_size;
     return RValue::get(emitBuiltinObjectSize(E->getArg(0), Type, ResType,
                                              /*EmittedE=*/nullptr, IsDynamic));
   }
   case Builtin::BI__builtin_prefetch: {
-    Value *Locality, *RW, *Address = EmitScalarExpr(E->getArg(0));
+    Value *Locality, *RW, *Data, *Address = EmitScalarExpr(E->getArg(0));
     // FIXME: Technically these constants should of type 'int', yes?
     RW = (E->getNumArgs() > 1) ? EmitScalarExpr(E->getArg(1)) :
       llvm::ConstantInt::get(Int32Ty, 0);
     Locality = (E->getNumArgs() > 2) ? EmitScalarExpr(E->getArg(2)) :
       llvm::ConstantInt::get(Int32Ty, 3);
-    Value *Data = llvm::ConstantInt::get(Int32Ty, 1);
+    Data = (E->getNumArgs() > 3) ? EmitScalarExpr(E->getArg(3)) :
+      llvm::ConstantInt::get(Int32Ty, 1);
     Function *F = CGM.getIntrinsic(Intrinsic::prefetch, Address->getType());
     return RValue::get(Builder.CreateCall(F, {Address, RW, Locality, Data}));
   }
   case Builtin::BI__builtin_readcyclecounter: {
     Function *F = CGM.getIntrinsic(Intrinsic::readcyclecounter);
     return RValue::get(Builder.CreateCall(F));
   }
   case Builtin::BI__builtin___clear_cache: {

clang/lib/Sema/SemaChecking.cpp

 }
 
 /// SemaBuiltinPrefetch - Handle __builtin_prefetch.
 // This is declared to take (const void*, ...) and can take two
 // optional constant int args.
 bool Sema::SemaBuiltinPrefetch(CallExpr *TheCall) {
   unsigned NumArgs = TheCall->getNumArgs();
 
-  if (NumArgs > 3)
+  if (NumArgs > 4)
     return Diag(TheCall->getEndLoc(),
                 diag::err_typecheck_call_too_many_args_at_most)
-           << 0 /*function call*/ << 3 << NumArgs << TheCall->getSourceRange();
+           << 0 /*function call*/ << 4 << NumArgs << TheCall->getSourceRange();
+
+  auto SemaBuiltinConstantArgRange =
+      [this, TheCall](int ArgNum, int Low, int High, int &Val) -> bool {
+    if (isConstantEvaluated())
+      return false;
+    llvm::APSInt Result;
+
+    // We can't check the value of a dependent argument.
+    Expr *Arg = TheCall->getArg(ArgNum);
+    if (Arg->isTypeDependent() || Arg->isValueDependent())
+      return false;
+
+    // Check constant-ness first.
+    if (SemaBuiltinConstantArg(TheCall, ArgNum, Result))
+      return true;
+
+    Val = Result.getSExtValue();
+    if (Val < Low || Val > High)
+      return Diag(TheCall->getBeginLoc(), diag::err_argument_invalid_range)
+             << Val << Low << High << Arg->getSourceRange();
+
+    return false;
+  };
 
   // Argument 0 is checked for us and the remaining arguments must be
   // constant integers.
+  int Vals[3] = {0, 0, 1};
   for (unsigned i = 1; i != NumArgs; ++i)
-    if (SemaBuiltinConstantArgRange(TheCall, i, 0, i == 1 ? 1 : 3))
+    if (SemaBuiltinConstantArgRange(i, 0, i == 2 ? 3 : 1, Vals[i - 1]))
       return true;
 
+  if (Vals[0] == 1 && Vals[2] == 0)
+    return Diag(TheCall->getEndLoc(),
+                diag::err_typecheck_call_inst_cache_must_read_only)
+           << TheCall->getSourceRange();
+
   return false;
 }
 
 /// SemaBuiltinArithmeticFence - Handle __arithmetic_fence.
 bool Sema::SemaBuiltinArithmeticFence(CallExpr *TheCall) {
   if (!Context.getTargetInfo().checkArithmeticFenceSupported())
     return Diag(TheCall->getBeginLoc(), diag::err_builtin_target_unsupported)
            << SourceRange(TheCall->getBeginLoc(), TheCall->getEndLoc());

clang/test/CodeGen/builtins-arm.c

 
 void prefetch(int i) {
   __builtin_arm_prefetch(&i, 0, 1);
   // CHECK: call {{.*}} @llvm.prefetch.p0(ptr %{{.*}}, i32 0, i32 3, i32 1)
 
   __builtin_arm_prefetch(&i, 1, 1);
   // CHECK: call {{.*}} @llvm.prefetch.p0(ptr %{{.*}}, i32 1, i32 3, i32 1)
 
-  __builtin_arm_prefetch(&i, 1, 0);
-  // CHECK: call {{.*}} @llvm.prefetch.p0(ptr %{{.*}}, i32 1, i32 3, i32 0)
+  __builtin_arm_prefetch(&i, 0, 0);
+  // CHECK: call {{.*}} @llvm.prefetch.p0(ptr %{{.*}}, i32 0, i32 3, i32 0)
 }
 
 void ldc(const void *i) {
   // CHECK: define{{.*}} void @ldc(ptr noundef %i)
   // CHECK: call void @llvm.arm.ldc(i32 1, i32 2, ptr %i)
   // CHECK-NEXT: ret void
   __builtin_arm_ldc(1, 2, i);
 }

clang/test/Sema/builtin-prefetch.c

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 void foo(void) {
   int a;
   __builtin_prefetch(&a);
   __builtin_prefetch(&a, 1);
-  __builtin_prefetch(&a, 1, 2);
-  __builtin_prefetch(&a, 1, 9, 3); // expected-error{{too many arguments to function}}
+  __builtin_prefetch(&a, 0, 2, 1);
+  __builtin_prefetch(&a, 1, 2, 0); // expected-error{{instruction cache must be read only}}
+  __builtin_prefetch(&a, 1, 9, 8, 3); // expected-error{{too many arguments to function}}
   __builtin_prefetch(&a, "hello", 2); // expected-error{{argument to '__builtin_prefetch' must be a constant integer}}
   __builtin_prefetch(&a, a, 2); // expected-error{{argument to '__builtin_prefetch' must be a constant integer}}
   __builtin_prefetch(&a, 2); // expected-error{{argument value 2 is outside the valid range [0, 1]}}
   __builtin_prefetch(&a, 0, 4); // expected-error{{argument value 4 is outside the valid range [0, 3]}}
   __builtin_prefetch(&a, -1, 4); // expected-error{{argument value -1 is outside the valid range [0, 1]}}
+  __builtin_prefetch(&a, 1, 2, 3); // expected-error{{argument value 3 is outside the valid range [0, 1]}}
 }

llvm/docs/LangRef.rst

 Arguments:
 """"""""""
 
 ``address`` is the address to be prefetched, ``rw`` is the specifier
 determining if the fetch should be for a read (0) or write (1), and
 ``locality`` is a temporal locality specifier ranging from (0) - no
 locality, to (3) - extremely local keep in cache. The ``cache type``
 specifies whether the prefetch is performed on the data (1) or
-instruction (0) cache. The ``rw``, ``locality`` and ``cache type``
-arguments must be constant integers.
+instruction (0) cache. When ``cache type`` is instruction, the ``rw``
+must be read. The ``rw``, ``locality`` and ``cache type`` arguments
+must be constant integers.
 
 Semantics:
 """"""""""
 
 This intrinsic does not modify the behavior of the program. In
 particular, prefetches cannot trap and do not produce a value. On
 targets that support this intrinsic, the prefetch can provide hints to
 the processor cache for better performance.

llvm/lib/IR/Verifier.cpp

case Intrinsic::gcread:
     Check(Call.getParent()->getParent()->hasGC(),
           "Enclosing function does not use GC.", Call);
     break;
   case Intrinsic::init_trampoline:
     Check(isa<Function>(Call.getArgOperand(1)->stripPointerCasts()),
           "llvm.init_trampoline parameter #2 must resolve to a function.",
           Call);
     break;
-  case Intrinsic::prefetch:
-    Check(cast<ConstantInt>(Call.getArgOperand(1))->getZExtValue() < 2,
-          "rw argument to llvm.prefetch must be 0-1", Call);
-    Check(cast<ConstantInt>(Call.getArgOperand(2))->getZExtValue() < 4,
-          "locality argument to llvm.prefetch must be 0-4", Call);
-    Check(cast<ConstantInt>(Call.getArgOperand(3))->getZExtValue() < 2,
-          "cache type argument to llvm.prefetch must be 0-1", Call);
+  case Intrinsic::prefetch: {
+    int RW = cast<ConstantInt>(Call.getArgOperand(1))->getZExtValue();
+    int Locality = cast<ConstantInt>(Call.getArgOperand(2))->getZExtValue();
+    int Data = cast<ConstantInt>(Call.getArgOperand(3))->getZExtValue();
+    Check(RW < 2, "rw argument to llvm.prefetch must be 0-1", Call);
+    Check(Locality < 4, "locality argument to llvm.prefetch must be 0-4", Call);
+    Check(Data < 2, "cache type argument to llvm.prefetch must be 0-1", Call);
+    Check(Data != 0 || RW != 1, "instruction cache must be read only", Call);
     break;
+  }
   case Intrinsic::stackprotector:
     Check(isa<AllocaInst>(Call.getArgOperand(1)->stripPointerCasts()),
           "llvm.stackprotector parameter #2 must resolve to an alloca.", Call);
     break;
   case Intrinsic::localescape: {
     BasicBlock *BB = Call.getParent();
     Check(BB == &BB->getParent()->front(),
           "llvm.localescape used outside of entry block", Call);

llvm/lib/Target/X86/X86Instr3DNow.td

 def PrefetchWT1Level : ImmLeaf<i32, [{
   return Imm < 3;
 }]>;
 
 let SchedRW = [WriteLoad] in {
 let Predicates = [Has3DNow, NoSSEPrefetch] in
 def PREFETCH : I3DNow<0x0D, MRM0m, (outs), (ins i8mem:$addr),
                       "prefetch\t$addr",
-                      [(prefetch addr:$addr, imm, imm, (i32 1))]>, TB;
+                      [(prefetch addr:$addr, imm, imm, (i32 imm))]>, TB;
 
 def PREFETCHW : I<0x0D, MRM1m, (outs), (ins i8mem:$addr), "prefetchw\t$addr",
                   [(prefetch addr:$addr, (i32 1), (i32 PrefetchWLevel), (i32 1))]>,
                   TB, Requires<[HasPrefetchW]>;
 
 def PREFETCHWT1 : I<0x0D, MRM2m, (outs), (ins i8mem:$addr), "prefetchwt1\t$addr",
                     [(prefetch addr:$addr, (i32 1), (i32 PrefetchWT1Level), (i32 1))]>,
                     TB, Requires<[HasPREFETCHWT1]>;
 }
 
 // "3DNowA" instructions
 defm PF2IW    : I3DNow_conv_rm_int<0x1C, "pf2iw", WriteCvtPS2I, "a">;
 defm PI2FW    : I3DNow_conv_rm_int<0x0C, "pi2fw", WriteCvtI2PS, "a">;
 defm PFNACC   : I3DNow_binop_rm_int<0x8A, "pfnacc", WriteFAdd, 0, "a">;
 defm PFPNACC  : I3DNow_binop_rm_int<0x8E, "pfpnacc", WriteFAdd, 0, "a">;
 defm PSWAPD   : I3DNow_conv_rm_int<0xBB, "pswapd", SchedWriteShuffle.MMX, "a">;

llvm/lib/Target/X86/X86InstrSSE.td

 
 //===----------------------------------------------------------------------===//
 // SSE 1 & 2 - Prefetch and memory fence
 //===----------------------------------------------------------------------===//
 
 // Prefetch intrinsic.
 let Predicates = [HasSSEPrefetch], SchedRW = [WriteLoad] in {
 def PREFETCHT0   : I<0x18, MRM1m, (outs), (ins i8mem:$src),
-    "prefetcht0\t$src", [(prefetch addr:$src, imm, (i32 3), (i32 1))]>, TB;
+    "prefetcht0\t$src", [(prefetch addr:$src, imm, (i32 3), imm)]>, TB;
 def PREFETCHT1   : I<0x18, MRM2m, (outs), (ins i8mem:$src),
-    "prefetcht1\t$src", [(prefetch addr:$src, imm, (i32 2), (i32 1))]>, TB;
+    "prefetcht1\t$src", [(prefetch addr:$src, imm, (i32 2), imm)]>, TB;
 def PREFETCHT2   : I<0x18, MRM3m, (outs), (ins i8mem:$src),
-    "prefetcht2\t$src", [(prefetch addr:$src, imm, (i32 1), (i32 1))]>, TB;
+    "prefetcht2\t$src", [(prefetch addr:$src, imm, (i32 1), imm)]>, TB;
 def PREFETCHNTA  : I<0x18, MRM0m, (outs), (ins i8mem:$src),
-    "prefetchnta\t$src", [(prefetch addr:$src, imm, (i32 0), (i32 1))]>, TB;
+    "prefetchnta\t$src", [(prefetch addr:$src, imm, (i32 0), imm)]>, TB;
 }
 
 // FIXME: How should flush instruction be modeled?
 let SchedRW = [WriteLoad] in {
 // Flush cache
 def CLFLUSH : I<0xAE, MRM7m, (outs), (ins i8mem:$src),
                "clflush\t$src", [(int_x86_sse2_clflush addr:$src)]>,
                PS, Requires<[HasSSE2]>;

llvm/test/CodeGen/SystemZ/prefetch-01.ll

 ; Test data prefetching.
 ;
 ; RUN: llc < %s -mtriple=s390x-linux-gnu | FileCheck %s
 
 declare void @llvm.prefetch(ptr, i32, i32, i32)
 
 @g = dso_local global [4096 x i8] zeroinitializer
 
 ; Check that instruction read prefetches are ignored.
 define dso_local void @f1(ptr %ptr) {
 ; CHECK-LABEL: f1:
 ; CHECK-NOT: %r2
 ; CHECK: br %r14
   call void @llvm.prefetch(ptr %ptr, i32 0, i32 0, i32 0)
   ret void
 }
 
-; Check that instruction write prefetches are ignored.
-define dso_local void @f2(ptr %ptr) {
-; CHECK-LABEL: f2:
-; CHECK-NOT: %r2
-; CHECK: br %r14
-  call void @llvm.prefetch(ptr %ptr, i32 1, i32 0, i32 0)
-  ret void
-}
-
 ; Check data read prefetches.
 define dso_local void @f3(ptr %ptr) {
 ; CHECK-LABEL: f3:
 ; CHECK: pfd 1, 0(%r2)
 ; CHECK: br %r14
   call void @llvm.prefetch(ptr %ptr, i32 0, i32 0, i32 1)
   ret void
 }
 

llvm/test/CodeGen/X86/prefetch.ll

 ; 3dnow by itself get you just the single prefetch instruction with no hints
 ; sse provides prefetch0/1/2/nta
 ; supporting prefetchw, but not 3dnow implicitly provides prefetcht0/1/2/nta regardless of sse setting as we need something to fall back to for the non-write hint.
 ; supporting prefetchwt1 implies prefetcht0/1/2/nta and prefetchw regardless of other settings. this allows levels for non-write and gives us an instruction for write+T0
 ; 3dnow prefetch instruction will only get used if you have no other prefetch instructions enabled
 
 ; rdar://10538297
 
-define void @t(ptr %ptr) nounwind  {
+define dso_local void @t(ptr %ptr) nounwind  {
 ; SSE-LABEL: t:
 ; SSE:       # %bb.0: # %entry
 ; SSE-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; SSE-NEXT:    prefetcht2 (%eax)
 ; SSE-NEXT:    prefetcht1 (%eax)
 ; SSE-NEXT:    prefetcht0 (%eax)
 ; SSE-NEXT:    prefetchnta (%eax)
 ; SSE-NEXT:    prefetcht2 (%eax)
 ; SSE-NEXT:    prefetcht1 (%eax)
 ; SSE-NEXT:    prefetcht0 (%eax)
 ; SSE-NEXT:    prefetchnta (%eax)
+; SSE-NEXT:    prefetcht1 (%eax)
+; SSE-NEXT:    prefetcht0 (%eax)
+; SSE-NEXT:    prefetcht1 t
+; SSE-NEXT:    prefetcht0 ext
 ; SSE-NEXT:    retl
 ;
 ; PRFCHWSSE-LABEL: t:
 ; PRFCHWSSE:       # %bb.0: # %entry
 ; PRFCHWSSE-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; PRFCHWSSE-NEXT:    prefetcht2 (%eax)
 ; PRFCHWSSE-NEXT:    prefetcht1 (%eax)
 ; PRFCHWSSE-NEXT:    prefetcht0 (%eax)
 ; PRFCHWSSE-NEXT:    prefetchnta (%eax)
 ; PRFCHWSSE-NEXT:    prefetchw (%eax)
 ; PRFCHWSSE-NEXT:    prefetchw (%eax)
 ; PRFCHWSSE-NEXT:    prefetchw (%eax)
 ; PRFCHWSSE-NEXT:    prefetchw (%eax)
+; PRFCHWSSE-NEXT:    prefetcht1 (%eax)
+; PRFCHWSSE-NEXT:    prefetcht0 (%eax)
+; PRFCHWSSE-NEXT:    prefetcht1 t
+; PRFCHWSSE-NEXT:    prefetcht0 ext
 ; PRFCHWSSE-NEXT:    retl
 ;
 ; PREFETCHWT1-LABEL: t:
 ; PREFETCHWT1:       # %bb.0: # %entry
 ; PREFETCHWT1-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; PREFETCHWT1-NEXT:    prefetcht2 (%eax)
 ; PREFETCHWT1-NEXT:    prefetcht1 (%eax)
 ; PREFETCHWT1-NEXT:    prefetcht0 (%eax)
 ; PREFETCHWT1-NEXT:    prefetchnta (%eax)
 ; PREFETCHWT1-NEXT:    prefetchwt1 (%eax)
 ; PREFETCHWT1-NEXT:    prefetchwt1 (%eax)
 ; PREFETCHWT1-NEXT:    prefetchw (%eax)
 ; PREFETCHWT1-NEXT:    prefetchwt1 (%eax)
+; PREFETCHWT1-NEXT:    prefetcht1 (%eax)
+; PREFETCHWT1-NEXT:    prefetcht0 (%eax)
+; PREFETCHWT1-NEXT:    prefetcht1 t
+; PREFETCHWT1-NEXT:    prefetcht0 ext
 ; PREFETCHWT1-NEXT:    retl
 ;
 ; 3DNOW-LABEL: t:
 ; 3DNOW:       # %bb.0: # %entry
 ; 3DNOW-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; 3DNOW-NEXT:    prefetch (%eax)
 ; 3DNOW-NEXT:    prefetch (%eax)
 ; 3DNOW-NEXT:    prefetch (%eax)
 ; 3DNOW-NEXT:    prefetch (%eax)
 ; 3DNOW-NEXT:    prefetchw (%eax)
 ; 3DNOW-NEXT:    prefetchw (%eax)
 ; 3DNOW-NEXT:    prefetchw (%eax)
 ; 3DNOW-NEXT:    prefetchw (%eax)
+; 3DNOW-NEXT:    prefetch (%eax)
+; 3DNOW-NEXT:    prefetch (%eax)
+; 3DNOW-NEXT:    prefetch t
+; 3DNOW-NEXT:    prefetch ext
 ; 3DNOW-NEXT:    retl
 entry:
   tail call void @llvm.prefetch( ptr %ptr, i32 0, i32 1, i32 1 )
   tail call void @llvm.prefetch( ptr %ptr, i32 0, i32 2, i32 1 )
   tail call void @llvm.prefetch( ptr %ptr, i32 0, i32 3, i32 1 )
   tail call void @llvm.prefetch( ptr %ptr, i32 0, i32 0, i32 1 )
   tail call void @llvm.prefetch( ptr %ptr, i32 1, i32 1, i32 1 )
   tail call void @llvm.prefetch( ptr %ptr, i32 1, i32 2, i32 1 )
   tail call void @llvm.prefetch( ptr %ptr, i32 1, i32 3, i32 1 )
   tail call void @llvm.prefetch( ptr %ptr, i32 1, i32 0, i32 1 )
+  tail call void @llvm.prefetch( ptr %ptr, i32 0, i32 2, i32 0 )
+  tail call void @llvm.prefetch( ptr %ptr, i32 0, i32 3, i32 0 )
+  tail call void @llvm.prefetch( ptr @t,   i32 0, i32 2, i32 0 )
+  tail call void @llvm.prefetch( ptr @ext, i32 0, i32 3, i32 0 )
   ret void
 }
 
+declare dso_local void @ext() nounwind
 declare void @llvm.prefetch(ptr, i32, i32, i32) nounwind