This is an archive of the discontinued LLVM Phabricator instance.

[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

Repository
rL LLVM

Event Timeline

lebedev.ri edited the summary of this revision. (Show Details)
  • Add vector tests (+undef)
  • Move out of test/CodeGen/MIR/Generic/ (they can't be there.), into test/CodeGen/{X86,AArch64}

I don't like this duplication, but not sure how to avoid it.

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.

test/CodeGen/AArch64/demorgan-extra.ll
4–6 ↗(On Diff #144018)

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. :)

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.

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.
That being said, the aarch64 mir output already looks somewhat alien, so yes, maybe i could test the final asm, for the most dumbest target with no special intrinsic sets.

test/CodeGen/AArch64/demorgan-extra.ll
4–6 ↗(On Diff #144018)

I think you meant to post that on test/CodeGen/*/demorgan.ll, i'd be ok with dropping them from dagcombine.
But not these demorgan-extra.ll tests. I need them.

lebedev.ri edited the summary of this revision. (Show Details)
  • Drop test/CodeGen/{X86,AArch64}/demorgan.ll.
  • Switch test/CodeGen/{X86,AArch64}/demorgan-extra.ll to testing the final assembly, not post machinecombiner MIR.
  • test nor pattern too
lebedev.ri planned changes to this revision.May 3 2018, 12:05 PM
spatel added a comment.May 5 2018, 8:53 AM

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.

spatel accepted this revision.May 5 2018, 8:53 AM

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.

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.
Thus i'm not sure i should land this as-is.

lebedev.ri abandoned this revision.Jun 21 2019, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2019, 8:51 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript