This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Instruction Selection to eliminate compare instructions
AbandonedPublic

Authored by lei on Mar 22 2017, 6:47 AM.

Details

Summary

Remove compare instructions from the code generation based on patterns outlined in the Compiler Writer's Guide (Appendix D).

D.1 Comparisons and Comparisons Against Zero
D.2 Negated Comparisons and Negated Comparisons Against Zero
D.3 ComparisonOperators

Diff Detail

Event Timeline

lei created this revision.Mar 22 2017, 6:47 AM
lei edited the summary of this revision. (Show Details)Mar 22 2017, 6:51 AM
lei edited the summary of this revision. (Show Details)Mar 22 2017, 6:55 AM
jtony added inline comments.Mar 22 2017, 7:01 AM
test/CodeGen/PowerPC/testComparesllgeuc.ll
19

I noticed that in some test cases we use generic registers (like here [[REG1]] ), and in some other test cases, we use the specific register (like r4,r5), I know this is caused by this patch are created by multiple people. But I think maybe it is worthwhile to use one format for all ? ( I personally prefer the auto generated one, but I am OK with both, as long as they are consistent)

lei added a reviewer: hfinkel.Mar 22 2017, 8:24 AM
lei added a subscriber: uweigand.
jtony added inline comments.Mar 22 2017, 8:55 AM
test/CodeGen/PowerPC/testComparesllgeuc.ll
19

I maybe not be clear enough, What I mean here is some test cases use generic names [[REG1:r[0-9]+]] (like this file testComparesllgeuc.ll), while the others just use whatever generated, like the setcc eqtest/CodeGen/PowerPC/testComparesieqsc.ll, there is no [[REG:r[0-9]+]] at all.

sfertile added inline comments.Mar 22 2017, 10:45 AM
test/CodeGen/PowerPC/testComparesllgeuc.ll
19

When writing these tests we want to use a specific register for the places where the abi dictates what register must be used, and variables for all other registers. For example in the next test (@test_llgeuc_sext):

subf [[REG1:r[0-9]+]], r4, r3 --> %a must be in register r3, and %b must be in register r4. The subf instruction isn't constrained to putting the result into a specific register so we shouldn't make any assumptions about which register it goes into as long as that result is used as the second operand of the rldicl instruction.

echristo edited edge metadata.Mar 30 2017, 11:46 AM

Hi Lei,

This patch is pretty enormous. Is there any way you can split this out into multiple smaller patches that each implement a specific thing? It'll optimize a bit better for reviewer time here.

Thanks!

-eric

nemanjai edited edge metadata.Mar 30 2017, 2:17 PM

Hi Lei,

This patch is pretty enormous. Is there any way you can split this out into multiple smaller patches that each implement a specific thing? It'll optimize a bit better for reviewer time here.

Thanks!

-eric

We initially discussed this a bit to try to come up with a way to split this out but didn't come up with particularly great ideas. The issues with splitting it up are:

  • The heart of the change is really the handling of ISD::SETCC nodes so is conceptually contained in one place
  • Majority of the changes are to the test cases

That being said, the least unreasonable way to split this up that I can suggest is the following:

  • Set of 4 patches corresponding to the 32/64 bit and ZEXT/SEXT functions that handle ISD::SETCC
  • Throughout the 4 patches, only use this for zero/sign extensions
  • In each patch, the remaining 3 functions are implemented but are stubs returning SDValue()
  • The fifth patch adds handling of ISD::SELECT
  • The sixth patch adds handling of ISD::SELECT_CC (including comparison operators)

All patches include test cases covering that patch

Let me know what you think.

SGTM. Anything you can do to help manage complexity helps :)

-eric

spatel added a subscriber: spatel.Apr 5 2017, 6:34 AM