[PATCH] D110177: [AArch64][SVE] Remove redundant PTEST following PNEXT/PFIRST

Wed Sep 22 05:53:13 PDT 2021

paulwalker-arm added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/sve-ptest-removal-pfirst.mir:3
+
+# Test instruction sequences where PTEST is redundant and thus gets removed.
+---
----------------
peterwaller-arm wrote:
> sdesmalen wrote:
> > Do these tests necessarily need to be MIR tests? The reason I ask is because I generally find them more difficult to read and would have thought some LLVM IR intrinsics would have been sufficient to prove an extra `ptest` is not added.
> Good question. These tests reuse the same form as the other tests for this. But I agree an IR test would be seem straightforward. I'll wait for anyone else to chime in with reasoning and if I don't hear anything I'll switch it on Monday next week. I have an open question in my mind of whether the other tests (llvm/test/CodeGen/AArch64/sve-ptest-removal-*.mir) should be migrated to IR tests too, as a separate commit.
My view is MIR tests are better at ensuring a specific piece of code within the code generator is exercised, which is a good fit for ensuring `isPTestLike` works as expected.

That said, this patch is only decorating more instructions with an existing property and so there isn't really any new functionality to exercise.  For that reason I see no problem with using an LLVM IR test to validate the newly decorated instructions but please don't convert all the existing MIR tests.

Perhaps just one of the `sve-ptest-removal-while##.mir` tests is sufficient and the others can be converted to LLVM IR tests.  Given the tests already exist I'm not sure what value there is in rewriting them, but that's your call.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110177