[PATCH] D48826: [AMDGPU] Add support for TFE/LWE in image intrinsics

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

nhaehnle added a comment.

Thank you, this looks much cleaner. I only have a small number of nitpicks left.



================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:885
       Info.opc = ISD::INTRINSIC_W_CHAIN;
-      Info.memVT = MVT::getVT(CI.getType());
+      Info.memVT = MVT::getVT(CI.getType(),true);
+      if (Info.memVT == MVT::Other) {
----------------
Space after the comma.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4668
+                                 MachineSDNode *Result,
+                                 SmallVector<EVT, 3> &ResultTypes,
+                                 bool IsTexFail, bool Unpacked, bool IsD16,
----------------
ResultTypes can be const, right? Use ArrayRef if yes, SmallVectorImpl otherwise.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4711-4713
+    DAG.ExtractVectorElements(CastRes, Elts, 0, DMaskPop);
+    for (SDValue &Elt : Elts) {
+      BVElts.push_back(Elt);
----------------
I think you can directly ExtractVectorElements into `BVElts` and get rid of `Elts` entirely.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:4980-4984
+    if (ResultTypes.size() == 3)
+      // Original result was aggregate type used for TexFailCtrl results
+      // The actual instruction returns as a vector type which has now been
+      // created. Remove the aggregate result.
+      ResultTypes.erase(&ResultTypes[1]);
----------------
Please put braces around the multi-line if "block" here.


Repository:
  rL LLVM

https://reviews.llvm.org/D48826