[PATCH] D55722: [DAGCombiner] scalarize binop followed by extractelement

Mon Dec 17 08:14:36 PST 2018

spatel added a comment.

In D55722#1332736 <https://reviews.llvm.org/D55722#1332736>, @uweigand wrote:

> Just looking at the SystemZ test cases:
>
> - The knownbits.ll case shows what might be an unintended consequence.  We have a vector "not" followed by a vector "and", which are merged into a single vector "and complement" operation, and this is then followed by an element extraction.  Your patch moves the "and" to the scalar side, but leaves the "not" a vector operation.  This means that we can now no longer combine the two into an "and complement".  Would it maybe be better to do the same transformation also for unary operations like "not"?

Thanks for explaining that output! Yes, I think we do want to extend this for unary ops like ISD::FNEG, but there's is no generic unary ISD::NOT node. We just do 'xor X, -1'. The same code example compiled for x86 behaves like we would expect - both logic ops are scalarized. So I looked at the debug output for this test with SystemZ target:

      t15: v4i32 = setcc t2, t4, setne:ch
    t17: i32 = extract_vector_elt t15, Constant:i32<3>
  t18: i32 = and t17, Constant:i32<1>

And then as legalization progresses:

          t21: v16i8 = SystemZISD::BYTE_MASK Constant:i32<65535>
        t22: v4i32 = bitcast t21
      t23: v4i32 = xor t19, t22
    t17: i32 = extract_vector_elt t23, Constant:i32<3>
  t18: i32 = and t17, Constant:i32<1>

So we do not recognize the t23 xor node as a 'not' because the constant value is hidden behind a bitcast of a target-specific constant. We can't handle that case here in generic combining without some kind of callback to know what to do with that constant node.

This does make me worry that I need to add an OpaqueConstant check into this somehow. Assuming that it is possible to have a build_vector that contains opaque constants?

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

https://reviews.llvm.org/D55722