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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
