Page MenuHomePhabricator

[Target][ARM] Add PerformVSELECTCombine for MVE Integer Ops
ClosedPublic

Authored by Pierre-vh on Apr 8 2020, 12:29 AM.

Details

Summary

This patch adds an implementation of PerformVSELECTCombine in the ARM DAG Combiner that transforms vselect(not(cond), lhs, rhs) into vselect(cond, rhs, lhs).

  • Isn't this supposed to be done in the (Target-Independent) DAGCombiner instead?
    • Yes, and I tried to do it with D77201, which caused issues with the X86 (AVX512) target (= it broke unit tests, which I unfortunately only noticed when it was upstream and I had to revert the change). The DAGCombiner doesn't optimize this for our cases as it doesn't allow truncation of the operands, so we have to do it here.
  • Why do it for MVE Integer Ops only?
    • It's the only target that was changed by this patch. I get the same result (tests all pass) by removing the if around the setTargetDAGCombine, so let's be cautious and just enable it for this scenario at first.
    • I believe all others cases are already handled by the DAGCombiner, it's just this case that needs special handling.

Diff Detail

Event Timeline

Pierre-vh created this revision.Apr 8 2020, 12:29 AM

There is the opposite transform in X86DAGCombiner? Hmm, I'm not sure if that's a great reason but I guess it's fine to put this here. I imagine the X86 code might have been different if the DAGCombine change had worked the other way all along. But it is probably simpler at least to do this here.

llvm/lib/Target/ARM/ARMISelLowering.cpp
949

You can change this.

11735–11736

It's probably best to put a check for if (!Subtarget->hasMVEIntegerOps()) return SDValue(); in case anyone in the future wants to add something Neon here too.

11738

I don't think this kind of comment adds a lot. It just says what the code says.

15267

You don't need to change this.

Pierre-vh updated this revision to Diff 256199.Apr 8 2020, 11:41 PM
  • Removed useless comment.
  • Now calls setTargetDAGCombine(ISD::VSELECT) everytime, but the PerformVSELECTCombine function will return early if MVE Integer Ops are not enabled.
Pierre-vh marked 4 inline comments as done.Apr 8 2020, 11:41 PM
dmgreen added inline comments.Apr 9 2020, 11:58 PM
llvm/lib/Target/ARM/ARMISelLowering.cpp
1462

This can still be in a hasMVE block. I was more talking about _if_ in the future someone added Neon code to it. It doesn't need it yet.

Pierre-vh updated this revision to Diff 257269.Apr 14 2020, 3:47 AM
  • Adding if (Subtarget->hasMVEIntegerOps()) before setTargetDAGCombine(ISD::VSELECT)
dmgreen accepted this revision.Apr 14 2020, 6:29 AM

LGTM

This revision is now accepted and ready to land.Apr 14 2020, 6:29 AM
This revision was automatically updated to reflect the committed changes.