Page MenuHomePhabricator

[PPC][DAGCombine] Convert SETCC to subtract when the result is zero extended
ClosedPublic

Authored by amehsan on Oct 3 2016, 8:03 PM.

Details

Summary

The code here can be extended. I post this patch right now, to get some feedback before adding those extensions.

When we see a SETCC whose only users are zero extend operations, we can replace it with a subtraction. This results in doing all calculation in GPRs and avoids CR use.

Extensions that I plan to look into:

1- Check if this works for any_extend/sign extend
2- Implement similar change for signed condition codes.
3- Look into handling equal and not equal cases, using cntlz
4- More tests should be added

Please let me know if there are other ways for extending this.

Diff Detail

Event Timeline

amehsan updated this revision to Diff 73390.Oct 3 2016, 8:03 PM
amehsan retitled this revision from to [PPC][DAGCombine] Convert SETCC to subtract when the result is zero extended.
amehsan updated this object.
amehsan added reviewers: hfinkel, kbarton, nemanjai.
amehsan added a subscriber: llvm-commits.
nemanjai added inline comments.Oct 13 2016, 3:31 PM
lib/Target/PowerPC/PPCISelLowering.cpp
9920

I'm not sure why the variable names are lowercase here.

9922

I think some sort of assert is in order here to ensure this function is called with the correct type of node.

9932

Could the expression size - 1 ever be zero, thereby making this a redundant shift operation?

9946

Perhaps a comment describing why it's not valid to do this prior to legalization.

9957

I think an assert should be added somewhere in this function as well to make sure you have the right node type or else this may trip an assert in a location that will make it difficult to debug.

9958

Do we really need this much indirection? Is there not a getValueType() member function in the SDValue operand? Will SDValue::getValueSizeInBits() not produce the right value?

9960

Shouldn't there be a check somewhere to ensure we're not in 32-bit mode?

kbarton requested changes to this revision.Oct 24 2016, 11:09 AM
kbarton edited edge metadata.
kbarton added inline comments.
lib/Target/PowerPC/PPCISelLowering.cpp
9920

Need comments here about the logic and what the assumptions are in terms of input parameters, etc.

9957

Some guidelines on the types of casts that can be used are here: http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

It's probably good to pick one, and then either put the appropriate checks earlier, or make sure to add comments stating it is assumed this is only called with nodes of a specific type.

This revision now requires changes to proceed.Oct 24 2016, 11:09 AM
amehsan added a comment.EditedOct 26 2016, 8:52 AM
This comment has been deleted.
amehsan added inline comments.Oct 26 2016, 12:14 PM
lib/Target/PowerPC/PPCISelLowering.cpp
9920

Will fix lower case var names.

9920

Will add comments.

9922

Will add

9932

No, this is going to be the size of largest legal integer.

9946

Will add

9957

Yes, this function is only called for ISD::SETCC. Will add more comments and assertions.

9958

Yes, I think SDValue::getValueSizeInBits will do what I need.

9960

This should be taken from DataLayout object. Will fix that.

amehsan updated this revision to Diff 75949.Oct 26 2016, 2:15 PM
amehsan edited edge metadata.

There is one comment not yet addressed. Size of latest legal integer is currently hardcoded. As I responded to that comment I will fix it. I want to look at this a bit more to see if it makes sense to add some extensions to it.

I have added more comments, so hopefully the code will be more clear.

nemanjai added inline comments.Oct 28 2016, 2:04 AM
lib/Target/PowerPC/PPCISelLowering.cpp
9946

I'm just curious why we don't see this shift in the test.

test/CodeGen/PowerPC/setcc-to-sub.ll
41

I'm kind of thinking that this should be a slightly stronger test - to show that the operands are swapped on the sub. The base register for the two loads is known, so maybe that can be used.

43

I don't think this is guaranteed to be R3 until the next instruction.

48

I think for completeness, you should include tests for the other two condition codes.

amehsan added a comment.EditedOct 28 2016, 6:04 AM

Thanks Nemanja for comments on the testcase.

I will revise the tests according to your comments. This patch originally being a draft to get feedback on the optimization approach, was not meant to provide finalized and exhaustive test.

amehsan added inline comments.Oct 28 2016, 6:14 AM
lib/Target/PowerPC/PPCISelLowering.cpp
9946

That should be an oversight. I will add this. I think I probably use CHECK-NEXT to make sure all the sequence is there.

test/CodeGen/PowerPC/setcc-to-sub.ll
41

Will look into it.

43

That is correct. Will change it. (Even though I hope our register allocation never makes that mistake :)

48

Sure . Will do.

amehsan updated this revision to Diff 76772.Nov 2 2016, 1:16 PM
amehsan edited edge metadata.

Completed the unit test and code. I decided that extending this for signed comparison is not necessary good, because two zero extends that we generate for unsigned comparison, will be sign extension for signed comparison. That means the signed version will have two more instructions. There might be still more useful cases, but I think they are different enough and we don't need to make them part of this patch.

Just finished lnt test and there are some failures. I will update when my investigation of the failures is done.

amehsan updated this revision to Diff 76843.Nov 3 2016, 5:28 AM

now fully tested with test-suite.

nemanjai added inline comments.Nov 3 2016, 10:39 AM
lib/Target/PowerPC/PPCISelLowering.cpp
9925

s/GenerateEquivalentSub/generateEquivalentSub

kbarton accepted this revision.Nov 10 2016, 11:46 AM
kbarton edited edge metadata.

Aside from some minor formatting of comments, this LGTM.

lib/Target/PowerPC/PPCISelLowering.cpp
9924

doxygen-style comments please

lib/Target/PowerPC/PPCISelLowering.h
973

Can you please convert to doxygen-style comments?

test/CodeGen/PowerPC/setcc-to-sub.ll
2

Is there any reason this has to be run on pwr8?

This revision is now accepted and ready to land.Nov 10 2016, 11:46 AM