As discussed in D46031, it seems DAGCombine should simplify using De Morgan laws.
test/Transforms/InstCombine/demorgan-extra.ll and test/CodeGen/{X86,AArch64}/demorgan-extra.ll are new and also 100% carbon copy
Paths
| Differential D46072
[DagCombine][InstCombine][NFC] De Morgan law tests AbandonedPublic Authored by lebedev.ri on Apr 25 2018, 10:40 AM.
Details Summary As discussed in D46031, it seems DAGCombine should simplify using De Morgan laws. test/Transforms/InstCombine/demorgan-extra.ll and test/CodeGen/{X86,AArch64}/demorgan-extra.ll are new and also 100% carbon copy
Diff Detail
Event Timelinelebedev.ri mentioned this in D46073: [DagCombine] De Morgan laws: 'nand' logic with an inverted operand. lebedev.ri added a child revision: D46073: [DagCombine] De Morgan laws: 'nand' logic with an inverted operand. Comment Actions
I don't like this duplication, but not sure how to avoid it. Comment Actions
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 Actions
One thing to note - i specifically want to see the post-dagcombine output, and do not want the latter passes to interfere, even potentially, thus it is outputted as MIR.
Comment Actions
Comment Actions LGTM. I think this covers typical and unusual cases that aren't visible anywhere else, so let's get this committed and off the dependency list. Comment Actions
These tests are only for the base pattern, *if* we want to evaluate the entirety of the problem (moving/fusing not), these tests won't be sufficient.
Revision Contents
Diff 144308 test/CodeGen/AArch64/demorgan-extra.ll
test/CodeGen/X86/demorgan-extra.ll
test/Transforms/InstCombine/demorgan-extra.ll
|
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. :)