[PATCH] D135633: [GlobalISel] Combine things like (z = x <= 0 ? z = x : z = 0) -> x & (x >> bw-1)

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:724-731
+  virtual bool hasAndNotCompare(LLT Ty) const { return false; }
+
+  /// LLT variant
+  virtual bool hasAndNot(LLT Ty) const {
+    // If the target has the more complex version of this operation, assume that
+    // it has this operation too.
+    return hasAndNotCompare(Ty);
----------------
Wouldn't the target just not add this combine if it didn't want it?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:6099-6102
+  // (z = x <= 0 ? z = x : z = 0) -> x & (x >> bw-1)
+  // (z = x < 0 ? z = x : z = 0) -> x & (x >> bw-1)
+  // (z = x >= 0 ? z = x : z = 0) -> x & ((~x) >> bw-1)
+  // (z = x > 0 ? z = x : z = 0) -> x & ((~x) >> bw-1)
----------------
These assignments on the right aren't part of the pattern and confusing


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:6113
+  LLT Ty = MRI.getType(Dst);
+  if (NeedsNot && !getTargetLowering().hasAndNot(Ty))
+    return false;
----------------
I guess this is complicated by this sub-variant, which would probably be uglier to split into a separate combine


================
Comment at: llvm/test/CodeGen/AArch64/combine-select-to-and-shift.mir:263
+    %select:_(s32) = G_SELECT %cmp(s1), %x, %one
+    $w0 = COPY %select(s32)
----------------
Vector case? Even if it's not directly legal I would expect this to combine to the vector for later scalarization if needed 


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

https://reviews.llvm.org/D135633