[SPIR] Prevent SPIR targets from using half conversion intrinsics

[smcgro]

Summary

The SPIR target currently allows for half precision floating point types to be emitted using the LLVM intrinsic functions which convert half types to floats and doubles. However, this is illegal in SPIR as the only intrinsic allowed by SPIR is memcpy, as per section 3 of the SPIR specification. Currently this is leading to an assert being hit in the Clang CodeGen when attempting to emit a constant or literal _Float16 type in a comparison operation on a SPIR or SPIR64 target. This assert stems from the CodeGen attempting to emit a constant half value as an integer because the backend has specified that it is using these half conversion intrinsics (which represents half as i16). This patch prevents SPIR targets from using these intrinsics by overloading the responsible target info method, marks SPIR targets as having a legal half type and provides additional regression testing for the _Float16 type on SPIR targets.

[SjoerdMeijer]

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.

lib/Basic/Targets/SPIR.h

50

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.

65

just a note: this is the only functional change, but you're testing a lot more in test/CodeGen/spir-half-type.cpp

test/CodeGen/spir-half-type.cpp

4

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.

5

nit: this comment exceeds 80 columns, same for the other comment below.

[smcgro]

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

lib/Basic/Targets/SPIR.h

50

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

65

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.

test/CodeGen/spir-half-type.cpp

4

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?

5

Fixed now, thanks

[SjoerdMeijer]

Looks OK to me.

test/CodeGen/spir-half-type.cpp

90

Nit: let one of these functions take a _Float16 function argument?

[smcgro]

Added a function parameter to one of the functions

[smcgro]

That's great @SjoerdMeijer , thanks for the review. I don't have commit access to clang so if there are no more reviews then it would be great if someone could commit this for me, thanks.

[SjoerdMeijer]

No problem, will commit this today.

lib/Basic/Targets/SPIR.h

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");
     TLSSupported = false;
     VLASupported = false;
     LongWidth = LongAlign = 64;
     AddrSpaceMap = &SPIRAddrSpaceMap;
     UseAddrSpaceMapMangling = true;
+    HasLegalHalfType = true;
     // Define available target features
     // These must be defined in sorted order!
     NoAsmVariants = true;
   }
 
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 
   bool hasFeature(StringRef Feature) const override {
     return Feature == "spir";
   }
 
+  // SPIR supports the half type and the only llvm intrinsic allowed in SPIR is
+  // memcpy as per section 3 of the SPIR spec.
+  bool useFP16ConversionIntrinsics() const override { return false; }
+
   ArrayRef<Builtin::Info> getTargetBuiltins() const override { return None; }
 
   const char *getClobbers() const override { return ""; }
 
   ArrayRef<const char *> getGCCRegNames() const override { return None; }
 
   bool validateAsmConstraint(const char *&Name,
                              TargetInfo::ConstraintInfo &info) const override {

test/CodeGen/spir-half-type.cpp

+// RUN: %clang_cc1 -O0 -triple spir -emit-llvm %s -o - | FileCheck %s
+// 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.
+
+// Previously attempting to use a constant _Float16 with a comparison
+// instruction when the target is spir or spir64 lead to an assert being hit.
+bool fcmp_const() {
+  _Float16 a = 0.0f16;
+  const _Float16 b = 1.0f16;
+
+  // CHECK-NOT: llvm.convert.to.fp16
+  // CHECK-NOT: llvm.convert.from.fp16
+
+  // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp olt half [[REG1]], 0xH3C00
+
+  // CHECK: [[REG2:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp olt half [[REG2]], 0xH4000
+
+  // CHECK: [[REG3:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp ogt half [[REG3]], 0xH3C00
+
+  // CHECK: [[REG4:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp ogt half [[REG4]], 0xH4200
+
+  // CHECK: [[REG5:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp oeq half [[REG5]], 0xH3C00
+
+  // CHECK: [[REG7:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp oeq half [[REG7]], 0xH4400
+
+  // CHECK: [[REG8:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp une half [[REG8]], 0xH3C00
+
+  // CHECK: [[REG9:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp une half [[REG9]], 0xH4500
+
+  // CHECK: [[REG10:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp ole half [[REG10]], 0xH3C00
+
+  // CHECK: [[REG11:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp ole half [[REG11]], 0xH4600
+
+  // CHECK: [[REG12:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp oge half [[REG12]], 0xH3C00
+
+  // CHECK: [[REG13:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: fcmp oge half [[REG13]], 0xH4700
+  return a < b || a < 2.0f16 || a > b || a > 3.0f16 || a == b || a == 4.0f16 ||
+         a != b || a != 5.0f16 || a <= b || a <= 6.0f16 || a >= b ||
+         a >= 7.0f16;
+}
+
+bool fcmp() {
+  _Float16 a = 0.0f16;
+  _Float16 b = 1.0f16;
+
+  // CHECK-NOT: llvm.convert.to.fp16
+  // CHECK-NOT: llvm.convert.from.fp16
+  // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG2:%.*]] = load half, half* %b, align 2
+  // CHECK-NEXT: fcmp olt half [[REG1]], [[REG2]]
+
+  // CHECK: [[REG3:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG4:%.*]] = load half, half* %b, align 2
+  // CHECK-NEXT: fcmp ogt half [[REG3]], [[REG4]]
+
+  // CHECK: [[REG5:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG6:%.*]] = load half, half* %b, align 2
+  // CHECK-NEXT: fcmp oeq half [[REG5]], [[REG6]]
+
+  // CHECK: [[REG7:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG8:%.*]] = load half, half* %b, align 2
+  // CHECK-NEXT: fcmp une half [[REG7]], [[REG8]]
+
+  // CHECK: [[REG7:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG8:%.*]] = load half, half* %b, align 2
+  // CHECK-NEXT: fcmp ole half [[REG7]], [[REG8]]
+
+  // CHECK: [[REG7:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG8:%.*]] = load half, half* %b, align 2
+  // CHECK-NEXT: fcmp oge half [[REG7]], [[REG8]]
+  return a < b || a > b || a == b || a != b || a <= b || a >= b;
+}
+
+_Float16 fadd() {
+  _Float16 a = 1.0f16;
+  const _Float16 b = 2.0f16;
+
+  // CHECK-NOT: llvm.convert.to.fp16
+  // CHECK-NOT: llvm.convert.from.fp16
+
+  // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG2:%.*]] = fadd half [[REG1]], 0xH4000
+  // CHECK-NEXT: [[REG3:%.*]] = fadd half [[REG2]], 0xH4200
+  // CHECK-NEXT: ret half [[REG3]]
+  return a + b + 3.0f16;
+}
+
+_Float16 fsub() {
+  _Float16 a = 1.0f16;
+  const _Float16 b = 2.0f16;
+
+  // CHECK-NOT: llvm.convert.to.fp16
+  // CHECK-NOT: llvm.convert.from.fp16
+
+  // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG2:%.*]] = fsub half [[REG1]], 0xH4000
+  // CHECK-NEXT: [[REG3:%.*]] = fsub half [[REG2]], 0xH4200
+  // CHECK-NEXT: ret half [[REG3]]
+  return a - b - 3.0f16;
+}
+
+// CHECK: define spir_func half @_Z4fmulDF16_(half %arg)
+_Float16 fmul(_Float16 arg) {
+  _Float16 a = 1.0f16;
+  const _Float16 b = 2.0f16;
+
+  // CHECK-NOT: llvm.convert.to.fp16
+  // CHECK-NOT: llvm.convert.from.fp16
+
+  // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG2:%.*]] = load half, half* %arg.addr, align 2
+  // CHECK-NEXT: [[REG3:%.*]] = fmul half [[REG1]], [[REG2]]
+  // CHECK-NEXT: [[REG4:%.*]] = fmul half [[REG3]], 0xH4000
+  // CHECK-NEXT: [[REG5:%.*]] = fmul half [[REG4]], 0xH4200
+  // CHECK-NEXT: ret half [[REG5]]
+  return a * arg * b * 3.0f16;
+}
+
+_Float16 fdiv() {
+  _Float16 a = 1.0f16;
+  const _Float16 b = 2.0f16;
+
+  // CHECK-NOT: llvm.convert.to.fp16
+  // CHECK-NOT: llvm.convert.from.fp16
+
+  // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+  // CHECK-NEXT: [[REG2:%.*]] = fdiv half [[REG1]], 0xH4000
+  // CHECK-NEXT: [[REG3:%.*]] = fdiv half [[REG2]], 0xH4200
+  // CHECK-NEXT: ret half [[REG3]]
+  return a / b / 3.0f16;
+}