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

[Stephen McGroarty via Phabricator via cfe-commits]

smcgro added a comment.

Thanks for the feedback @SjoerdMeijer I've updated the commit message and responded to the comments



================
Comment at: lib/Basic/Targets/SPIR.h:50
     UseAddrSpaceMapMangling = true;
+    HasLegalHalfType = true;
     // Define available target features
----------------
SjoerdMeijer wrote:
> 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.
Yeah that's true that it doesn't affect the assert being hit, I included it as it is true for the SPIR target as the SPIR targets //do// have a legal half type which is brings the SPIR target file in line with what the specification says.  If this is problematic or it needs a different set of tests I can remove it


================
Comment at: lib/Basic/Targets/SPIR.h:65
+  // memcpy as per section 3 of the SPIR spec.
+  bool useFP16ConversionIntrinsics() const override { return false; }
+
----------------
SjoerdMeijer wrote:
> just a note: this is the only functional change, but you're testing a lot more in test/CodeGen/spir-half-type.cpp
That's true that the fix is only for the assert being hit when attempting to emit comparison operators. The additional tests I added are mainly to prevent regressions in the future since the cross section of the _Float16 C/C++ type and the SPIR  target doesn't have any tests at the moment so we'd like to be sure that it will always be compiled correctly. In addition it helps make sure that this change doesn't have any unintended consequences with any of the other operators.


================
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.
----------------
SjoerdMeijer wrote:
> 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. 
My thought process was that we should have a test just for this _Float16/SPIR cross section as that is where this issue emerged from. These are specific to SPIR insofar that the comparison operators previously wouldn't compile (on Debug or Release with asserts builds) so their inclusion is necessary to check that this now compiles. As I mentioned above the others are there to prevent future regressions on this target and to check that this change doesn't have any unintended side effects with the other operators. I agree that the smoke tests should have the operator tests but we do need these operations to be tested on the SPIR target as well to make sure they compile. Perhaps including these operations in those float16 specific regression tests should be a separate issue however, as it unrelated to this SPIR change?


================
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.
+
----------------
SjoerdMeijer wrote:
> nit: this comment exceeds 80 columns, same for the other comment below.
Fixed now, thanks


Repository:
  rC Clang

https://reviews.llvm.org/D48188