[PATCH] D46072: [DagCombine][InstCombine][NFC] De Morgan law tests

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

> I think for DAGCombine we should be testing MIR output.

That's more limited & stable, but there's a tradeoff. It means that we're requiring reviewers/contributors to understand another syntax/mini-language in addition to the IR input and their favorite target's asm output. I think that's why, in x86 at least, we still mostly have tests output to asm. Also with small & focused tests, the likelihood of something later in the pipeline altering the expected output is low, so we're fine testing x86 asm as the output.



================
Comment at: test/CodeGen/AArch64/demorgan-extra.ll:4-6
+; There is a identical twin test in test/Transforms/InstCombine/demorgan-extra.ll
+; Please keep them in sync! :)
+
----------------
I understand that we want to coordinate instcombine and DAGCombine here, but I don't agree with this requirement. In particular, it doesn't make sense to have a big chunk of tests with illegal types in the DAG. (It doesn't add much to instcombine at this point either - I'm confident that APInt works as advertised, and we have plenty of tests for weird types already.)

So several tests here don't add enough value to justify their cost IMO (similar comment as in D46008) because we try *not* to produce those types in the first place. It's nice to have a bit of coverage to see that we don't miscompile, but that's about it.

So again, this is just my opinion, but I'd rather see more economical use of tests and less tests overall. I do appreciate the thoroughness and test comments though. :)


Repository:
  rL LLVM

https://reviews.llvm.org/D46072