[PATCH] Adding _mm_prefetch and other builtins

Warren Hunt whunt at google.com
Wed Feb 19 15:28:03 PST 2014


  Committed as r201734.


================
Comment at: lib/Sema/SemaChecking.cpp:700
@@ +699,3 @@
+  switch (BuiltinID) {
+  case X86::BI_mm_prefetch:
+    return SemaBuiltinMMPrefetch(TheCall);
----------------
Hans Wennborg wrote:
> i'm probably missing something, but why don't we have checks for the other builtins too? or is it just that we want to do some extra intelligent checks here?
We need to check that the int argument is known at compile time to be in [0,3]

================
Comment at: lib/Sema/SemaChecking.cpp:1978
@@ +1977,3 @@
+
+  if (NumArgs > 2)
+    return Diag(TheCall->getLocEnd(),
----------------
Hans Wennborg wrote:
> we've declared in the .def file that it takes two parameters.. do we still have to check that manually?
We do not, I'll remove the test.  The updated lit tests verify that this is still checked elsewhere.

================
Comment at: test/Headers/mmprefetch.c:3
@@ +2,3 @@
+#include <mmintrin.h>
+void _mm_prefetch(char const*, int);
+
----------------
Hans Wennborg wrote:
> maybe add a comment that this tests that redeclaration works?
done.

================
Comment at: test/Headers/mmprefetch.c:6
@@ +5,3 @@
+void f() {
+  static char a = 0;
+  _mm_prefetch(&a, 0);
----------------
Hans Wennborg wrote:
> I guess the static isn't important here?
correct. changed to be an argument to f.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:4790
@@ +4789,3 @@
+  case X86::BI_mm_prefetch: {
+    Value *Locality, *RW, *Address = EmitScalarExpr(E->getArg(0));
+    RW = ConstantInt::get(Int32Ty, 0);
----------------
Hans Wennborg wrote:
> nit pick: I think it would be more in line with existing style to declare Locality and RW where they're initialized below
Done.  I agree.  I have no idea why it was the way it was.

================
Comment at: include/clang/Basic/BuiltinsX86.def:31
@@ +30,3 @@
+BUILTIN(_InterlockedDecrement, "LiLiD*", "n")
+BUILTIN(_InterlockedExchangeAdd, "LiLiD*Li", "n")
+
----------------
David Majnemer wrote:
> These should not be available in all language modes. Perhaps use `LANGBUILTIN` with `ALL_MS_LANGUAGES`?
Done.  x86 builtins don't support LANGBUILTIN so I had to move them to Builtins.def.  There is an appropriate place to put them.

================
Comment at: test/CodeGen/ms-builtins.c:14
@@ +13,3 @@
+  __noop();
+  __debugbreak();
+};
----------------
Hans Wennborg wrote:
> The __noop() and __debugbreak() seem redundant for the test..
Yes, all of this was part of me debugging before I realized we needed -fms-extentions to get any of it to work.  I forgot to remove it.


http://llvm-reviews.chandlerc.com/D2822



More information about the cfe-commits mailing list