[PATCH] D85479: [SVE][VLS] Don't combine logical AND.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

I do not understand the new tests.  At first glance they look identical to those in sve-fixed-length-int-log.ll and when I test them against HEAD they pass as is (i.e. without the fix to performANDCombine).  I suspect you need a more complex test to exercise the buggy code path.

Whilst discussing the testing I'd prefer to keep the SVE fixed length tests under the sve-fixed-length-* namespace and given the change I doubt you need test files for multiple variants of SVE.  If if works for SVE256 then why would it break for larger implementations.  You could follow a similar style as sve-fixed-length-subvector.ll or sve-fixed-length-int-log.ll (note the GE LE difference).



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11113
+  // when dealing with vectors wider than 128 bits.
+  if (VT.getSizeInBits() > 128)
+    return SDValue();
----------------
Does the combine also apply to 64-bit vectors?  I know the code it correct but the comment might need updating.  If it helps I normally use "NEON sized vectors" or "NEON vectors" to cover all the cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85479