[PATCH] D57389: [X86] Improve use of SHLD/SHRD

Sat Feb 9 12:52:06 PST 2019

RKSimon added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:37054
+        ShAmt1Op1 = ShAmt1Op1.getOperand(0);
+      }
       if (ShAmt1Op1.getOpcode() == ISD::TRUNCATE)
----------------
RKSimon wrote:
> craig.topper wrote:
> > deadalnix wrote:
> > > craig.topper wrote:
> > > > deadalnix wrote:
> > > > > RKSimon wrote:
> > > > > > Maybe use DAG.GetDemandedBits ?
> > > > > It's not obvious to me that it would work. The and opcode itself uses it, so there is no need to redo it here. We also need to know if masking took place to know what we can accept as LHS for the sub opcode.
> > > > I know this code (Bits - 1) check is used in other places earlier in this function, but is that valid for i16 SHLD/SHRD? SHLD/SHRD hardware mask shift amount to 5 bits on i16/i32 and 6 bits on i64. 
> > > I do not know if this is valid for i16. If that isn't, this code is bogous already. Would you have a test case I can rely on to do this ?
> > @RKSimon what are you thoughts on the existing uses of (Bits - 1) in this function for the i16 case? Those seem wrong to me.
> Yes I think its wrong - we're most likely being saved by the fact that its so tricky to create i16 double shifts from code (PR35155). As I said I'd much prefer to kill all this code and move it to FSHL/FSHR in DAGCombine (PR40081) - I'll take a look.
Unravelling all of this code is proving trickier than I hoped due to various custom/legalization issues. I've committed rL353626 which folds to FSHL/FSHR and a fix for the i16 issue.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57389/new/

https://reviews.llvm.org/D57389