SHL instruction crash in CodeGen - Fixed

[delena]

Summary

Shift-left (ISD::SHL) operation crashes on "DAG Legalization" phase.
https://llvm.org/bugs/show_bug.cgi?id=29058.

While node legalization we tried to legalize its operands. If an operand node is replaced during legalization it may kill the user.

[RKSimon]

Any chance that the test case can be simplified (i.e. de-bugpointified) a little?

../lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1073 ↗

(On Diff #70360)

If it worth calling Node->getOperand(0) and Node->getOperand(1) only once?

1080 ↗

(On Diff #70360)

So are you saying that if SAO == Node->getOperand(1) then its guaranteed to be already legal? Do we need to assert this?

[delena]

In D24244#535645, @RKSimon wrote:

Any chance that the test case can be simplified (i.e. de-bugpointified) a little?

I did my best. I worked hard after the bugpoint gave up.

../lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1080 ↗

(On Diff #70360)

No. I say that SAO node will legalized in the next round. Legalization of more than one node together may crash.

[RKSimon]

LGTM with a few minors

../lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1080 ↗

(On Diff #70360)

OK, please make that more clear in the comment.

../test/CodeGen/X86/shl-crash-on-legalize.ll

10 ↗

(On Diff #70360)

var_366 doesn't appear to be referenced.

31 ↗

(On Diff #70360)

Could rename the variables to make it more readable?

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

#endif
   if (SimpleFinishLegalizing) {
     SDNode *NewNode = Node;
     switch (Node->getOpcode()) {
     default: break;
     case ISD::SHL:
     case ISD::SRL:
     case ISD::SRA:
     case ISD::ROTL:
-    case ISD::ROTR:
+    case ISD::ROTR: {
       // Legalizing shifts/rotates requires adjusting the shift amount
       // to the appropriate width.
-      if (!Node->getOperand(1).getValueType().isVector()) {
-        SDValue SAO =
-          DAG.getShiftAmountOperand(Node->getOperand(0).getValueType(),
-                                    Node->getOperand(1));
-        HandleSDNode Handle(SAO);
-        LegalizeOp(SAO.getNode());
-        NewNode = DAG.UpdateNodeOperands(Node, Node->getOperand(0),
-                                         Handle.getValue());
-      }
-      break;
+      SDValue Op0 = Node->getOperand(0);
+      SDValue Op1 = Node->getOperand(1);
+      if (!Op1.getValueType().isVector()) {
+        SDValue SAO = DAG.getShiftAmountOperand(Op0.getValueType(), Op1);
+        // The getShiftAmountOperand() may create a new operand node or
+        // return the existing one. If new operand is created we need
+        // to update the parent node.
+        // Do not try to legalize SAO here! It will be automatically legalized
+        // in the next round.
+        if (SAO != Op1)
+          NewNode = DAG.UpdateNodeOperands(Node, Op0, SAO);
+      }
     case ISD::SRL_PARTS:
     case ISD::SRA_PARTS:
-    case ISD::SHL_PARTS:
+    case ISD::SHL_PARTS: {
       // Legalizing shifts/rotates requires adjusting the shift amount
       // to the appropriate width.
-      if (!Node->getOperand(2).getValueType().isVector()) {
-        SDValue SAO =
-          DAG.getShiftAmountOperand(Node->getOperand(0).getValueType(),
-                                    Node->getOperand(2));
-        HandleSDNode Handle(SAO);
-        LegalizeOp(SAO.getNode());
-        NewNode = DAG.UpdateNodeOperands(Node, Node->getOperand(0),
-                                         Node->getOperand(1),
-                                         Handle.getValue());
-      }
-      break;
+      SDValue Op0 = Node->getOperand(0);
+      SDValue Op1 = Node->getOperand(1);
+      SDValue Op2 = Node->getOperand(2);
+      if (!Op2.getValueType().isVector()) {
+        SDValue SAO = DAG.getShiftAmountOperand(Op0.getValueType(), Op2);
+        // The getShiftAmountOperand() may create a new operand node or
+        // return the existing one. If new operand is created we need
+        // to update the parent node.
+        if (SAO != Op2)
+          NewNode = DAG.UpdateNodeOperands(Node, Op0, Op1, SAO);
+      }
     }
 
     if (NewNode != Node) {
       ReplaceNode(Node, NewNode);
       Node = NewNode;
     }
     switch (Action) {
     case TargetLowering::Legal:

llvm/trunk/test/CodeGen/X86/shl-crash-on-legalize.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+;RUN: llc < %s | FileCheck %s
+
+; This test case failed on legalization of "shl" node. PR29058.
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@structMember = external local_unnamed_addr global i64, align 8
+
+; Function Attrs: norecurse nounwind uwtable
+define i32 @_Z3foov() local_unnamed_addr #0 {
+; CHECK-LABEL: _Z3foov:
+; CHECK:       # BB#0: # %entry
+; CHECK-NEXT:    movq %rax, {{.*}}(%rip)
+; CHECK-NEXT:    retq
+entry:
+  %bool_1 = icmp ne i8 undef, 0
+  %bool_2 = icmp eq i8 undef, 0
+  %0 = select i1 %bool_2, i32 2147483646, i32 undef
+  %or_1 = select i1 %bool_1, i32 undef, i32 -1
+  %shl_1 = shl i32 %0, %or_1
+  %conv = zext i32 %shl_1 to i64
+  store i64 %conv, i64* @structMember, align 8
+  %tmp = select i1 %bool_2, i32 2147483646, i32 undef
+  %lnot = icmp eq i8 undef, 0
+  %or_2 = select i1 %lnot, i32 -1, i32 undef
+  %shl_2 = shl i32 %tmp, %or_2
+  ret i32 %shl_2
+}
+
+attributes #0 = { norecurse nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+