[PATCH] D49026: [AMDGPU] New tbuffer intrinsics

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Some smaller nitpicks, but mostly looks good to me!



================
Comment at: lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:3556
+    return MatchOperand_NoMatch;
+  auto Format = Dfmt | Nfmt << 4;
+  Operands.push_back(
----------------
Maybe an out-of-bounds check here?


================
Comment at: lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:4800
   {"inst_offset", AMDGPUOperand::ImmTyInstOffset, false, nullptr},
-  {"dfmt",    AMDGPUOperand::ImmTyDFMT, false, nullptr},
-  {"nfmt",    AMDGPUOperand::ImmTyNFMT, false, nullptr},
+  {"dfmt",    AMDGPUOperand::ImmTyFORMAT, false, nullptr},
   {"glc",     AMDGPUOperand::ImmTyGLC, true, nullptr},
----------------
Don't we still need the "nfmt" line here as well, in case the order is reversed? I see you've added a test for that, but I don't actually understand how that test is passing...


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5565
+  } else if ((C1 = dyn_cast<ConstantSDNode>(N0)))
+    N0 = SDValue(nullptr, 0);
+
----------------
Just `SDValue()` should do the trick.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5578
+      auto OverflowVal = DAG.getConstant(Overflow, DL, MVT::i32);
+      if (!N0.getNode())
+        N0 = OverflowVal;
----------------
Can just be `!N0`, like below.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5590
+    C1 = cast<ConstantSDNode>(DAG.getConstant(0, DL, MVT::i32));
+  return std::pair<SDValue, SDValue>(N0, SDValue(C1, 0));
+}
----------------
I think we should be able to just say `return {N0, SDValue(C1, 0)};` here.


Repository:
  rL LLVM

https://reviews.llvm.org/D49026