[cfe-commits] [Patch] SPIR32/SPIR64 targets in Clang

[Tanya Lattner]

Looks fine to me.

-Tanya

On Dec 11, 2012, at 6:49 AM, "Benyei, Guy" <guy.benyei at intel.com> wrote:

> 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:eli.friedman at gmail.com] 
> Sent: Tuesday, December 11, 2012 01:18
> To: Benyei, Guy
> Cc: cfe-commits at cs.uiuc.edu
> Subject: Re: [cfe-commits] [Patch] SPIR32/SPIR64 targets in Clang
> 
> On Sun, Dec 9, 2012 at 1:30 AM, Benyei, Guy <guy.benyei at intel.com> 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.
> <SPIRtarget3.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits