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