[PATCH] D42088: [x86] shrink 'and' immediate values by setting the high bits (PR35907)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2471
+
+  SDLoc DL(And);
+  SDValue NewMask = CurDAG->getConstant(NegVal, DL, VT);
----------------
spatel wrote:
> craig.topper wrote:
> > One of these days we should really decide between 'DL' and 'dl'...
> I thought 'dl' was a holdover from the days when variables were lower-case? I'd be happy to reformat the whole file. :)
It feels like everytime I touch DAGCombiner or X86ISelLowering I have to figure out which name is already used in the function I'm editing.

And to really make it confusing, there are a couple places in X86ISelLowering that have DataLayout in DL and an SDLoc in dl.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2473
+  SDValue NewMask = CurDAG->getConstant(NegVal, DL, VT);
+  SDValue NewAnd = CurDAG->getNode(ISD::AND, DL, VT, And0, NewMask);
+  ReplaceNode(And, NewAnd.getNode());
----------------
spatel wrote:
> craig.topper wrote:
> > Just to make sure I understand. If the new mask is all ones, getNode constant folds the AND away leaving the constant disconnected. But isel will notice the constant is unused and not select it? Or maybe isel doesn't see the constant at all because it wasn't there when the selection process started?
> > 
> > Though if we can prove the upper bits are 0 and the result is all ones why didn't simplify demanded bits during DAG combine already remove the AND. I'm just trying to understand what happened on the tests in divide-by-constant.ll
> Yes, getNode has a fold for 'and X, -1' --> X. 
> 
> If we look at 'test3', we have this going in:
>           t4: i32 = AssertZext t2, ValueType:ch:i8
>         t41: i32 = mul t4, Constant:i32<171>
>       t38: i32 = srl t41, Constant:i8<9>
>     t39: i32 = and t38, Constant:i32<127>
> 
> So getNode() returns the srl (t38), that replaces the and (t39), and we select that srl now:
> 
>   Selecting: t39: i32 = and t38, Constant:i32<127>
>   Creating constant: t43: i32 = Constant<-1>
>   ISEL: Starting pattern match on root node: t38: i32 = srl t41, Constant:i8<9>
> 
I guess I'm slightly worried about someone coming along and adding code in front of the call to  Select that checks the opcode of the root node and now we've switched to a different root. We do have such code now, but it doesn't apply here and is admittedly a hack.

```
      if (Node->isStrictFPOpcode())
        Node = CurDAG->mutateStrictFPToFP(Node);

      Select(Node);
```

It means we can only match strict FP nodes when they are the root node and their opcode gets converted. But it might have otherwise been save to fold a vector select to use a masked op.

But I guess in general its probably not safe to look at the root node before the select. So this may be an unfounded fear.


https://reviews.llvm.org/D42088