[PATCH] D20644: [NVPTX] Added NVVMIntrRange pass

[Justin Lebar via llvm-commits]

jlebar added a comment.

\o/


================
Comment at: lib/Target/NVPTX/NVPTXTargetMachine.cpp:181
@@ -178,2 +180,3 @@
   PM.add(createNVVMReflectPass());
+  PM.add(createNVVMIntrRangePass(Subtarget.getSmVersion()));
 }
----------------
I have no idea if this is the right way to pass this information to the pass.  Seems reasonable to me, though.

================
Comment at: lib/Target/NVPTX/NVVMIntrRange.cpp:10
@@ +9,3 @@
+//
+// This pass adds appropriate !range metadata for calls of NVVM
+// intrinsics that return limited range of values.
----------------
calls to

================
Comment at: lib/Target/NVPTX/NVVMIntrRange.cpp:11
@@ +10,3 @@
+// This pass adds appropriate !range metadata for calls of NVVM
+// intrinsics that return limited range of values.
+//
----------------
a limited range

================
Comment at: lib/Target/NVPTX/NVVMIntrRange.cpp:36
@@ +35,3 @@
+     unsigned x, y, z;
+   } block, grid;
+
----------------
I think at least a comment, and possibly more descriptive names (blockMaxes / gridMaxes?) would be helpful to readers.

Actually, since you don't use the fact that these are structs at all, maybe just six member variables would be clearer.  maxBlockX, maxThreadZ, etc.  What do you think?

================
Comment at: lib/Target/NVPTX/NVVMIntrRange.cpp:60
@@ +59,3 @@
+// Adds the passed-in range information as metadata to the passed-in
+// call instruction.
+static bool addRangeMetadata(int Low, int High, CallInst *C) {
----------------
Maybe indicate that the range is [Low, High)?

================
Comment at: lib/Target/NVPTX/NVVMIntrRange.cpp:66
@@ +65,3 @@
+      ConstantAsMetadata::get(llvm::ConstantInt::get(Int32Ty, Low)),
+      ConstantAsMetadata::get(llvm::ConstantInt::get(Int32Ty, High))};
+  C->setMetadata(llvm::LLVMContext::MD_range,
----------------
We're already using namespace llvm, so don't need "llvm::" here.

================
Comment at: test/CodeGen/NVPTX/intrinsic-old.ll:10
@@ -4,2 +9,3 @@
 define ptx_device i32 @test_tid_x() {
 ; CHECK: mov.u32 %r{{[0-9]+}}, %tid.x;
+; RANGE: call i32 @llvm.ptx.read.tid.x(), !range ![[BLK_IDX_XY:[0-9]+]]
----------------
Probably shouldn't do it in this patch, but FYI if you add CHECK-LABEL: @foo in front of each function, that will make it a lot easier to debug if it fails.  CHECK-LABEL essentially partitions the checks.

Although I think it applies only to CHECKs, not e.g. RANGEs.

================
Comment at: test/CodeGen/NVPTX/intrinsic-old.ll:314
@@ +313,2 @@
+; RANGE-DAG: ![[GRID_SIZE_YZ]] = !{i32 1, i32 65536}
+
----------------
It looks like we're covering everything other than warpSize -- can we add that one too, for completeness?


http://reviews.llvm.org/D20644