This is an archive of the discontinued LLVM Phabricator instance.

[X86] Improve combineVectorShiftImm
ClosedPublic

Authored by foad on Apr 2 2020, 5:45 AM.

Details

Summary

Fold (shift (shift X, C2), C1) -> (shift X, (C1 + C2)) for logical as
well as arithmetic shifts. This is needed to prevent regressions from
an upcoming funnel shift expansion change.

While we're here, fold (VSRAI -1, C) -> -1 too.

Diff Detail

Event Timeline

foad created this revision.Apr 2 2020, 5:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2020, 5:45 AM
RKSimon added inline comments.Apr 2 2020, 7:04 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
40883

ISD::isBuildVectorAll* leaves UNDEFs in there - we must generate the constant

foad marked an inline comment as done.Apr 2 2020, 7:41 AM
foad added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
40883

I don't quite understand why. If there are undefs in there then we will effectively be folding (VSRAI undef, C) -> undef (for some lanes). Isn't that allowed?

RKSimon added inline comments.Apr 2 2020, 8:46 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
40883

More clearly we're guaranteeing that for logical shifts that the upper bits will be zero. I'd recommend playing it safe as we've been hit by these things before (PR43159/PR43177) as SimplifyDemandedBits becomes more capable:

// Shift N0 by zero -> N0.
if (!ShiftVal)
  return N0;
// Shift zero -> zero.
if (ISD::isBuildVectorAllZeros(N0.getNode()))
  return DAG.getConstant(0, SDLoc(N), VT);
// (VSRAI -1, C) -> -1
if (!LogicalShift && ISD::isBuildVectorAllOnes(N0.getNode()))
  return DAG.getConstant(-1, SDLoc(N), VT);
foad updated this revision to Diff 254546.Apr 2 2020, 9:31 AM

Address review comments about undef.

foad updated this revision to Diff 254548.Apr 2 2020, 9:33 AM

Fix comments.

RKSimon accepted this revision.Apr 3 2020, 2:58 AM

LGTM

This revision is now accepted and ready to land.Apr 3 2020, 2:58 AM

reverse ping?

This revision was automatically updated to reflect the committed changes.