This is an archive of the discontinued LLVM Phabricator instance.

[X86] Simplify cmp-zext-constant DAG patterns.
AbandonedPublic

Authored by bryant on Jul 21 2016, 6:32 AM.

Details

Summary

This cuts down on the spurious generation of MOV32rr and should be correct for the E, NE, P, NP condition codes. However, I am unsure how useful this might be since movl uses zero micro-ops.

I've added nadav since s/he appears in grep -i x86 CODE_OWNERS.txt.

Diff Detail

Event Timeline

bryant updated this revision to Diff 64867.Jul 21 2016, 6:32 AM
bryant retitled this revision from to [X86] Simplify cmp-zext-constant DAG patterns..
bryant updated this object.
bryant set the repository for this revision to rL LLVM.
bryant updated this object.Jul 21 2016, 6:35 AM
bryant added a reviewer: nadav.
bryant added a subscriber: nadav.
bryant removed a subscriber: nadav.
bryant updated this revision to Diff 64874.Jul 21 2016, 6:48 AM
bryant removed rL LLVM as the repository for this revision.

Added test case.

bryant updated this revision to Diff 64876.Jul 21 2016, 6:51 AM

Revert accident deletion

bryant set the repository for this revision to rL LLVM.Jul 21 2016, 6:52 AM
RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
test/CodeGen/X86/cmp-zext-combine.ll
21

Please can you simplify the test to focus just on the combine in question. And preferably add a wider range of tests touching different comparisons / integer sizes (inc ones that shouldn't combine).

If possible use utils/update_llc_test_checks.py to generate full code output (you will need to add check prefixes to the 32/64 tests)

spatel added inline comments.Jul 21 2016, 12:24 PM
lib/Target/X86/X86ISelLowering.cpp
30277–30282

Usually (always?) you would start at the outer op/instruction/node of a pattern that you want to match. Is there some reason not to implement this starting in combineSetCC() rather than combineX86Cmp()?

spatel edited edge metadata.Jul 21 2016, 12:28 PM

Just noticed that llvm-commits is not subscribed to this review. It's probably best to abandon this review and start again with llvm-commits subscribed from the outset.

bryant added inline comments.Jul 21 2016, 2:29 PM
lib/Target/X86/X86ISelLowering.cpp
30277–30282
  • in order for this transformation to occur, there must be an ISD::ZERO_EXTEND present in the inner node.
  • often, however, the zext doesn't appear until DAGCombiner generates it somewhere down the line in the combine process -- after the corresponding parent setcc has been touched by combineSetCC/combineX86SetCC;
    • ISD::ZERO_EXTEND is set to Expand in this file, although it won't matter much since there are many other patterns that need DAGCombiner to distill into a zext.
  • when DAGCombiner combines, it re-adds to the work list only the direct users of the zext node. this would be the cmp but not the setcc.

Upon closer inspection, I've discovered that the true root of the problem lies in DAGCombiner failing to recognize and combine (srl (shl, c), c). So I will be re-submitting the fix there instead.

RKSimon edited edge metadata.Jul 23 2016, 5:15 AM

Abandon this given D22726?

bryant abandoned this revision.Jul 24 2016, 7:49 PM