This is an archive of the discontinued LLVM Phabricator instance.

X86: Do not select X86 custom vector nodes if operand types don't match
ClosedPublic

Authored by MatzeB on Apr 20 2015, 11:55 AM.

Details

Summary

X86ISD::ADDSUB, X86ISD::(F)HADD, X86ISD::(F)HSUB should not be selected
if the operand types do not match the result type because vector type
legalization cannot deal with this for custom nodes.

Testcase X86ISD::ADDSUB is attached. I could not create a testcase for
the FHADD/FHSUB cases because of: https://llvm.org/bugs/show_bug.cgi?id=23296

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 24040.Apr 20 2015, 11:55 AM
MatzeB retitled this revision from to X86: Do not select X86 custom vector nodes if operand types don't match.
MatzeB updated this object.
MatzeB edited the test plan for this revision. (Show Details)
MatzeB added reviewers: chandlerc, qcolombet.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: Unknown Object (MLST).
ab added a subscriber: ab.Apr 20 2015, 1:23 PM

For the missing testcase, one hack you can use to force an additional DAGCombine round is to introduce illegal vector ops (so there's another vector ops legalization phase, with the associated combining round), since IIRC the BUILD_VECTOR->SHUFP lowering happens in the final ops legalization phase, after vector ops legalization. See r235243, where I used ctpop. That's icky though, and shows that our AddUsersToWorklist tricks aren't enough (but that's a discussion for another day ;)

lib/Target/X86/X86ISelLowering.cpp
5272–5285 ↗(On Diff #24040)

Should you put the checks outside the if/else? Something like

if (V0.getVT() != VT || V1.getVT != VT)
  return false;

Not sure it's better.

andreadb accepted this revision.Apr 20 2015, 2:49 PM
andreadb added a reviewer: andreadb.
andreadb added a subscriber: andreadb.

Hi Matthias,
the patch looks good to me. I only added a very minor comment in the test (see below).

As a side note: I think that the major problem here is that the logic that matches horizontal add/sub operations is run as part of a target specific combine on BUILD_VECTOR nodes.
This is sub-optimal and may lead to a premature selection of target specific nodes.
As a long term fix, in bug 23296 I suggested to see if it is possible/reasonable to move all the horizontal add/sub selection logic from performBUILD_VECTORCombine in (a function called by) LowerBUILD_VECTOR.

Thanks,
Andrea

test/CodeGen/X86/sse3-avx-addsub-2.ll
318 ↗(On Diff #24040)

You can remove the '#0'. That attribute set is not defined.

This revision is now accepted and ready to land.Apr 20 2015, 2:49 PM

Thanks for the reviews. I decided against pulling out the getVT() == VT checks, as then I would need extra checks to guard against Undef which doesn't make the code nicer IMO.

Changing horizontal add/sub and ADDSUB matching to not be a DAGCombine but a lowering makes perfect sense, I'll try to change the code to that.

This revision was automatically updated to reflect the committed changes.