[PATCH] D48188: [SPIR] Prevent SPIR targets from using half conversion intrinsics

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

I know very little about SPIR, and Initially didn't understand this:

> The SPIR target currently allows for half precision floating point types to use the LLVM intrinsic functions to convert to floats and doubles. This is illegal in SPIR as the only intrinsic allowed by SPIR is memcpy ...

until I looked at the implementation what you're trying to achieve here. Perhaps you can make the commit message a bit more descriptive and specific.



================
Comment at: lib/Basic/Targets/SPIR.h:50
     UseAddrSpaceMapMangling = true;
+    HasLegalHalfType = true;
     // Define available target features
----------------
It doesn't hurt to set this, but you're not using it so you could omit it. I had to introduce this to deal differently with half types depending on architecture extensions, but don't you think have this problem.


================
Comment at: lib/Basic/Targets/SPIR.h:65
+  // memcpy as per section 3 of the SPIR spec.
+  bool useFP16ConversionIntrinsics() const override { return false; }
+
----------------
just a note: this is the only functional change, but you're testing a lot more in test/CodeGen/spir-half-type.cpp


================
Comment at: test/CodeGen/spir-half-type.cpp:3
+// RUN: %clang_cc1 -O0 -triple spir64 -emit-llvm %s -o - | FileCheck %s
+
+// This file tests that using the _Float16 type with the spir target will not use the llvm intrinsics but instead will use the half arithmetic instructions directly.
----------------
I think you need one reproducer to test:

// CHECK-NOT: llvm.convert.from.fp16

The other tests, like all the compares are valid tests, but not related to this change, and also not specific to SPIR. I put my _Float16 "smoke tests" in test/CodeGenCXX/float16-declarations.cpp, perhaps you can move some of these generic tests there because I for example see I didn't add any compares there. 


================
Comment at: test/CodeGen/spir-half-type.cpp:4
+
+// This file tests that using the _Float16 type with the spir target will not use the llvm intrinsics but instead will use the half arithmetic instructions directly.
+
----------------
nit: this comment exceeds 80 columns, same for the other comment below.


Repository:
  rC Clang

https://reviews.llvm.org/D48188