Hi Eli,
Thanks for the review.

Attached a fixed patch, please review.

> +    virtual bool hasFeature(StringRef Feature) const {
> +      return Feature == "spir";
> +    }
>
> What's the point of this?

This one is quite the same as in every virtual architecture, including 
PNaCl/le32. I guess it is a convinient way to find out if the current target 
implements this kind of abstraction.

thanks
    Guy Benyei


-----Original Message-----
From: Eli Friedman [mailto:[email protected]] 
Sent: Tuesday, December 11, 2012 01:18
To: Benyei, Guy
Cc: [email protected]
Subject: Re: [cfe-commits] [Patch] SPIR32/SPIR64 targets in Clang

On Sun, Dec 9, 2012 at 1:30 AM, Benyei, Guy <[email protected]> wrote:
>
> As I've got no response up until now, I'm re-sending this patch - 
> removed some target defines that probably should be defined in the 
> pre-processor initialization.
>
>
>
> I think this is a fairly simple patch.

Sorry about the delay; I was out last week.

+      assert(getTriple().getOS() == llvm::Triple::UnknownOS &&
+        "SPIR target must use unknown OS");
+      assert(getTriple().getEnvironment() ==
llvm::Triple::UnknownEnvironment &&
+        "SPIR target must use unknown environment type");

Don't assert something we don't enforce.  If you really want to enforce it, 
check before the "new SPIR32TargetInfo(T)".

+    virtual bool hasFeature(StringRef Feature) const {
+      return Feature == "spir";
+    }

What's the point of this?

+    virtual const char *getVAListDeclaration() const {
+      return "";
+    }

How did this sneak in?

Index: test/CodeGenOpenCL/spir32_target.cl
===================================================================
--- test/CodeGenOpenCL/spir32_target.cl (revision 0)
+++ test/CodeGenOpenCL/spir32_target.cl (revision 0)
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 %s -triple "spir-unknown-unknown" -emit-llvm -o -
| FileCheck %s
+
+// CHECK: target triple = "spir-unknown-unknown"
+
+typedef struct {
+  char c;
+  void *v;
+  void *v2;
+} my_st;
+
+kernel void foo(global long *arg) {
+  arg[0] = sizeof(my_st);
+//CHECK: store i64 12, i64 addrspace(1)*

We generally prefer to write this sort of test using _Static_Assert or 
equivalent.

-Eli
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Attachment: SPIRtarget3.patch
Description: SPIRtarget3.patch

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to