[PATCH] D9751: Add HSAIL target

[Matthias Braun]

MatzeB added a subscriber: MatzeB.
MatzeB added a comment.

Just some codingstyle issues I found while skimming over a few files


================
Comment at: lib/Target/HSAIL/AMDOpenCLKernenv.h:10-12
@@ +9,5 @@
+//
+// \file
+// \brief Declare OpenCL dispatch-specific constants that are passed
+//        as additional arguments (the "kernenv") to the HSAIL kernel.
+//
----------------
Needs three slashes. There are similar problems in many of the other files.

================
Comment at: lib/Target/HSAIL/AMDOpenCLKernenv.h:16-17
@@ +15,4 @@
+
+#ifndef __AMD_OPENCL_KERNENV_H__
+#define __AMD_OPENCL_KERNENV_H__
+
----------------
Should be changed to LIB_TARGET_HSAIL_AMDOPENCLKERNELENV_H. There are similar issues in other headers.

================
Comment at: lib/Target/HSAIL/HSAIL.h:25-43
@@ +24,21 @@
+
+#define HSAIL_MAJOR_VERSION 3
+#define HSAIL_MINOR_VERSION 1
+#define HSAIL_REVISION_NUMBER 104
+#define HSAIL_20_REVISION_NUMBER 88
+#define ARENA_SEGMENT_RESERVED_UAVS 12
+#define DEFAULT_ARENA_UAV_ID 8
+#define DEFAULT_RAW_UAV_ID 7
+#define GLOBAL_RETURN_RAW_UAV_ID 11
+#define HW_MAX_NUM_CB 8
+#define MAX_NUM_UNIQUE_UAVS 8
+
+// The next two values can never be zero, as zero is the ID that is
+// used to assert against.
+#define DEFAULT_LDS_ID 1
+#define DEFAULT_GDS_ID 1
+#define DEFAULT_SCRATCH_ID 1
+#define DEFAULT_VEC_SLOTS 8
+
+#define OCL_DEVICE_ALL 0xFFFFF
+
----------------
Better use static const unsigned or enum values so debuggers can pick those constants up as well.

================
Comment at: lib/Target/HSAIL/HSAIL.h:217-221
@@ +216,7 @@
+
+// Target architectures to optimize for
+enum OptimizeForTargetArch {
+  GENERIC, // No target specific flavor
+  SI       // Optimize fot Southern Islands family
+};
+
----------------
Could use doxygen comments here:
/// Target architectures ...
enum ... {
  GENERIC, ///< No target specific flavor
  ...


================
Comment at: lib/Target/HSAIL/HSAILAlwaysInlinePass.cpp:24
@@ +23,3 @@
+
+class HSAILAlwaysInline : public ModulePass {
+
----------------
Would be nice to name the file like the class declared inside.

================
Comment at: test/CodeGen/HSAIL/128bit-kernel-args.ll:1
@@ +1,2 @@
+; RUN: llc -march=hsail < %s | FileCheck -check-prefix=HSAIL -check-prefix=FUNC %s
+
----------------
Many of the tests define new prefixes instead of just using "CHECK" should be unnecessary if you just test for 1 variant anyway.


http://reviews.llvm.org/D9751