[llvm] r356979 - [X86] In matchBitExtract, place all of the new nodes before Node's position in the DAG for the topological sort.

[Roman Lebedev via llvm-commits]

Thanks, and sorry.
I missed that *all* the new nodes should simply go before
the original root node, with no weird ordering between them.

Roman.

On Tue, Mar 26, 2019 at 8:30 AM Craig Topper via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
> Author: ctopper
> Date: Mon Mar 25 22:31:32 2019
> New Revision: 356979
>
> URL: http://llvm.org/viewvc/llvm-project?rev=356979&view=rev
> Log:
> [X86] In matchBitExtract, place all of the new nodes before Node's position in the DAG for the topological sort.
>
> We were using OrigNBits, but that put all the nodes before the node we used to start the control computation. This caused some node earlier than the sequence we inserted to be selected before the sequence we created. We want our new sequence to be selected first since it depends on OrigNBits.
>
> I don't have a test case. Found by reviewing the code.
>
> Modified:
>     llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp
>
> Modified: llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp?rev=356979&r1=356978&r2=356979&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp Mon Mar 25 22:31:32 2019
> @@ -3008,19 +3008,18 @@ bool X86DAGToDAGISel::matchBitExtract(SD
>      assert(XVT == MVT::i64 && "Expected truncation from i64");
>    }
>
> -  SDValue OrigNBits = NBits;
>    // Truncate the shift amount.
>    NBits = CurDAG->getNode(ISD::TRUNCATE, DL, MVT::i8, NBits);
> -  insertDAGNode(*CurDAG, OrigNBits, NBits);
> +  insertDAGNode(*CurDAG, SDValue(Node, 0), NBits);
>
>    // Insert 8-bit NBits into lowest 8 bits of 32-bit register.
>    // All the other bits are undefined, we do not care about them.
>    SDValue ImplDef = SDValue(
>        CurDAG->getMachineNode(TargetOpcode::IMPLICIT_DEF, DL, MVT::i32), 0);
> -  insertDAGNode(*CurDAG, OrigNBits, ImplDef);
> +  insertDAGNode(*CurDAG, SDValue(Node, 0), ImplDef);
>    NBits = CurDAG->getTargetInsertSubreg(X86::sub_8bit, DL, MVT::i32, ImplDef,
>                                          NBits);
> -  insertDAGNode(*CurDAG, OrigNBits, NBits);
> +  insertDAGNode(*CurDAG, SDValue(Node, 0), NBits);
>
>    if (Subtarget->hasBMI2()) {
>      // Great, just emit the the BZHI..
> @@ -3028,10 +3027,10 @@ bool X86DAGToDAGISel::matchBitExtract(SD
>        // But have to place the bit count into the wide-enough register first.
>        SDValue ImplDef = SDValue(
>            CurDAG->getMachineNode(TargetOpcode::IMPLICIT_DEF, DL, XVT), 0);
> -      insertDAGNode(*CurDAG, OrigNBits, ImplDef);
> +      insertDAGNode(*CurDAG, SDValue(Node, 0), ImplDef);
>        NBits = CurDAG->getTargetInsertSubreg(X86::sub_32bit, DL, XVT, ImplDef,
>                                              NBits);
> -      insertDAGNode(*CurDAG, OrigNBits, NBits);
> +      insertDAGNode(*CurDAG, SDValue(Node, 0), NBits);
>      }
>
>      SDValue Extract = CurDAG->getNode(X86ISD::BZHI, DL, XVT, X, NBits);
> @@ -3050,7 +3049,7 @@ bool X86DAGToDAGISel::matchBitExtract(SD
>    // This makes the low 8 bits to be zero.
>    SDValue C8 = CurDAG->getConstant(8, DL, MVT::i8);
>    SDValue Control = CurDAG->getNode(ISD::SHL, DL, MVT::i32, NBits, C8);
> -  insertDAGNode(*CurDAG, OrigNBits, Control);
> +  insertDAGNode(*CurDAG, SDValue(Node, 0), Control);
>
>    // If the 'X' is *logically* shifted, we can fold that shift into 'control'.
>    if (X.getOpcode() == ISD::SRL) {
> @@ -3068,17 +3067,17 @@ bool X86DAGToDAGISel::matchBitExtract(SD
>
>      // And now 'or' these low 8 bits of shift amount into the 'control'.
>      Control = CurDAG->getNode(ISD::OR, DL, MVT::i32, Control, ShiftAmt);
> -    insertDAGNode(*CurDAG, OrigNBits, Control);
> +    insertDAGNode(*CurDAG, SDValue(Node, 0), Control);
>    }
>
>    // But have to place the 'control' into the wide-enough register first.
>    if (XVT != MVT::i32) {
>      SDValue ImplDef =
>          SDValue(CurDAG->getMachineNode(TargetOpcode::IMPLICIT_DEF, DL, XVT), 0);
> -    insertDAGNode(*CurDAG, OrigNBits, ImplDef);
> +    insertDAGNode(*CurDAG, SDValue(Node, 0), ImplDef);
>      Control = CurDAG->getTargetInsertSubreg(X86::sub_32bit, DL, XVT, ImplDef,
>                                              Control);
> -    insertDAGNode(*CurDAG, OrigNBits, Control);
> +    insertDAGNode(*CurDAG, SDValue(Node, 0), Control);
>    }
>
>    // And finally, form the BEXTR itself.
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits