[PATCH] D55444: AMDGPU: Fix DPP combiner

[Connor Abbott via Phabricator via llvm-commits]

cwabbott added a comment.

I figured it would be a little easier if I looked at these cases by myself. It turns out there are more problems with `isIdentityValue`, including some correctness issues. After fixing these, everything works correctly now.



================
Comment at: lib/Target/AMDGPU/GCNDPPCombine.cpp:265
+  case AMDGPU::V_SUBREV_I32_e32:
+  case AMDGPU::V_MIN_U32_e32:
+    if (OldOpnd->getImm() == 0)
----------------
You have min and max mixed up... the identity for min is the maximum possible value, not the minimum. The same goes for signed and unsigned min/max. Also, you can add V_XOR with an identity value of 0 here.


================
Comment at: lib/Target/AMDGPU/GCNDPPCombine.cpp:271
   case AMDGPU::V_MAX_U32_e32:
-    if (OldOpndValue.getImm() == std::numeric_limits<uint32_t>::max())
-      return OldOpndVGPR;
+    if (OldOpnd->getImm() ==
+        static_cast<int64_t>(std::numeric_limits<uint32_t>::max()))
----------------
It turns out that LLVM sign-extends the immediate to 64 bits sometime during instruction selection, which means this won't work for any MIR generated by LLVM IR. We should be ignoring the high 32 bits when doing all these comparisons, since they're irrelevant.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55444/new/

https://reviews.llvm.org/D55444