Page MenuHomePhabricator

[X86] Use setcc ISD opcode for AVX512 integer comparisons all the way to isel

Authored by craig.topper on Feb 21 2018, 9:29 PM.



I don't believe there is any real reason to have separate X86 specific opcodes for vector compares. Setcc has the same behavior just uses a different encoding for the condition code.

I had to change the CondCodeAction for SETLT and SETLE to prevent some transforms from changing SETGT lowering. This requires D43607 to make isCondCodeLegal not return true for 'Custom'.

Diff Detail


Event Timeline

craig.topper created this revision.Feb 21 2018, 9:29 PM
craig.topper added inline comments.Feb 21 2018, 9:44 PM
6934 ↗(On Diff #135358)

This isn't strictly related but is needed to prevent a regression. The handling for the case when "ZeroExtended" is null and "Ld" is non-null doesn't work correctly.

In the Ld case, the VT.getVectorElementType() and EltType are the same. But we need the element type we're zero extending from.

I think previously target independent DAG combine ran reduceBuildVecExtToExtBuildVec after setcc was turned into pcmpm. This pushed the code to use "ZeroExtended" case. But now we don't change the setcc, so the target independent DAG combine doesn't get a chance to run. Leaving us with the "Ld" case.

3180 ↗(On Diff #135358)

This is needed because the explicit CondCode match doesn't seem to increase the pattern complexity the way an explicit CondCode match does.

143 ↗(On Diff #135358)

I'm not sure why this changed. I think its because vector lowering didn't change anything. It used to change setcc to pcmpm. But it doesn't now, so the post vector lowering DAG combine doesn't run. And there's a target independent DAG combine, reduceBuildVecExtToExtBuildVec, that is only allowed to run after legalize vector ops. And if the post vector ops dag combine doesn't run, then the build vector is lowered before the last DAG combine runs so reduceBuildVecExtToExtBuildVec doesn't get called.

zvi added a subscriber: zvi.Apr 22 2018, 12:38 PM

Is this patch pending for review? If yes, can you please rebase it on ToT?

@craig.topper Are you still looking at this?

Rebase. The test changes are all the same as before I believe

RKSimon accepted this revision.Jun 20 2018, 6:12 AM

LGTM with a couple of minors

3239 ↗(On Diff #151995)

Add a comment explaining the AddedComplexity?

3180 ↗(On Diff #135358)

Can you put this in a comment here?

143 ↗(On Diff #135358)

Isn't this actually better?

This revision is now accepted and ready to land.Jun 20 2018, 6:12 AM
craig.topper added inline comments.Jun 20 2018, 1:36 PM
143 ↗(On Diff #135358)

Yes it is better. I was just explaining that it was an accident and would probably easily break on a larger test case.

This revision was automatically updated to reflect the committed changes.