This is an archive of the discontinued LLVM Phabricator instance.

[X86] Improve uniform funnelshift/rotation amount handling
ClosedPublic

Authored by RKSimon on Feb 6 2022, 12:57 PM.

Details

Summary

To find uniform shift/rotation amounts, we currently use SelectionDAG::getSplatValue which creates a node that extracts the scalar value from the source vector, this makes it more difficult for later combines to remove the extraction and stay on the SIMD unit, and can be a problem when the scalar type is illegal (i.e. i64 vs v2i64 on 32-bit targets).

This patch begins to use SelectionDAG::getSplatSourceVector (which SelectionDAG::getSplatValue uses internally) and adds a new variant of getTargetVShiftNode that takes the source vector and the splat index, and adjusts the vector in place to create the zero-extended value suitable for the SSE PSLL/PSRL/PSRA uniform instructions.

I'm still addressing a number of regressions when used for normal vector shifts, so I've just handled the funnelshift/rotation lowering for this first patch. I can then focus on the yak shaving (SimplifyDemandedBits/Elts in particular) necessary to always use SelectionDAG::getSplatSourceVector.

Diff Detail

Event Timeline

RKSimon created this revision.Feb 6 2022, 12:57 PM
RKSimon requested review of this revision.Feb 6 2022, 12:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2022, 12:57 PM

and can be a problem when the scalar type is illegal (i.e. i64 vs v2i64 on 32-bit targets).

Didn't find a case for v2i64. Do we need to add one?

and can be a problem when the scalar type is illegal (i.e. i64 vs v2i64 on 32-bit targets).

Didn't find a case for v2i64. Do we need to add one?

We're currently protected against this with a legality check inside X86TargetLowering::isSplatValueForTargetNode that can be removed once we don't use SelectionDAG::getSplatValue anymore (and I'm hoping to remove getSplatValue entirely) - if we remove it it does currently crash in a number of places that lowers vector shifts later. It's a good example of why I'm having to piecemeal transition the shift-by-scalar lowering, there's a number of crashes and regressions that all need to be cleaned up in the right order :-(

switch (Opc) {
case X86ISD::VBROADCAST:
case X86ISD::VBROADCAST_LOAD:
  // TODO: Permit vXi64 types on 32-bit targets.
  if (isTypeLegal(Op.getValueType().getVectorElementType())) {
    UndefElts = APInt::getNullValue(NumElts);
    return true;
  }
  return false;
}
pengfei accepted this revision.Feb 12 2022, 5:56 AM

LGTM with some nits.

llvm/lib/Target/X86/X86ISelLowering.cpp
25713–25714

Move it close to its use?

25725

Can we handle for 64 bit now? Should simply add an assert instead?

25761–25762

Should move comment in line 25758 here?

This revision is now accepted and ready to land.Feb 12 2022, 5:56 AM

and can be a problem when the scalar type is illegal (i.e. i64 vs v2i64 on 32-bit targets).

Didn't find a case for v2i64. Do we need to add one?

We're currently protected against this with a legality check inside X86TargetLowering::isSplatValueForTargetNode that can be removed once we don't use SelectionDAG::getSplatValue anymore (and I'm hoping to remove getSplatValue entirely) - if we remove it it does currently crash in a number of places that lowers vector shifts later. It's a good example of why I'm having to piecemeal transition the shift-by-scalar lowering, there's a number of crashes and regressions that all need to be cleaned up in the right order :-(

switch (Opc) {
case X86ISD::VBROADCAST:
case X86ISD::VBROADCAST_LOAD:
  // TODO: Permit vXi64 types on 32-bit targets.
  if (isTypeLegal(Op.getValueType().getVectorElementType())) {
    UndefElts = APInt::getNullValue(NumElts);
    return true;
  }
  return false;
}

Got it, thanks!

This revision was landed with ongoing or failed builds.Feb 12 2022, 6:48 AM
This revision was automatically updated to reflect the committed changes.