[PATCH] D29292: [SelectionDAG] Fix for PR30775: Assertion `NodeToMatch->getOpcode() != ISD::DELETED_NODE && "NodeToMatch was removed partway through selection"' failed.
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 30 11:37:27 PST 2017
Alexey Bataev via Phabricator <reviews at reviews.llvm.org> writes:
> ABataev created this revision.
>
> NodeToMatch can be modified during matching, but the code does not
> handle this situation.
>
>
> https://reviews.llvm.org/D29292
>
> Files:
> lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> test/CodeGen/X86/dag-update-nodetomatch.ll
>
> Index: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> +++ lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> @@ -2832,14 +2832,15 @@
> /// for this.
> class MatchStateUpdater : public SelectionDAG::DAGUpdateListener
> {
> - SmallVectorImpl<std::pair<SDValue, SDNode*> > &RecordedNodes;
> - SmallVectorImpl<MatchScope> &MatchScopes;
> + SDNode *&NodeToMatch;
> + SmallVectorImpl<std::pair<SDValue, SDNode *>> &RecordedNodes;
> + SmallVectorImpl<MatchScope> &MatchScopes;
> public:
> - MatchStateUpdater(SelectionDAG &DAG,
> - SmallVectorImpl<std::pair<SDValue, SDNode*> > &RN,
> - SmallVectorImpl<MatchScope> &MS) :
> - SelectionDAG::DAGUpdateListener(DAG),
> - RecordedNodes(RN), MatchScopes(MS) { }
> + MatchStateUpdater(SelectionDAG &DAG, SDNode *&NodeToMatch,
> + SmallVectorImpl<std::pair<SDValue, SDNode *>> &RN,
> + SmallVectorImpl<MatchScope> &MS)
> + : SelectionDAG::DAGUpdateListener(DAG), NodeToMatch(NodeToMatch),
> + RecordedNodes(RN), MatchScopes(MS) {}
>
> void NodeDeleted(SDNode *N, SDNode *E) override {
> // Some early-returns here to avoid the search if we deleted the node or
> @@ -2849,6 +2850,9 @@
> // update listener during matching a complex patterns.
> if (!E || E->isMachineOpcode())
> return;
> + // Check if NodeToMatch was updated.
> + if (N == NodeToMatch)
> + NodeToMatch = E;
> // Performing linear search here does not matter because we almost never
> // run this code. You'd have to have a CSE during complex pattern
> // matching.
> @@ -3141,7 +3145,7 @@
> // consistent.
> std::unique_ptr<MatchStateUpdater> MSU;
> if (ComplexPatternFuncMutatesDAG())
> - MSU.reset(new MatchStateUpdater(*CurDAG, RecordedNodes,
> + MSU.reset(new MatchStateUpdater(*CurDAG, NodeToMatch, RecordedNodes,
This looks correct, but I don't like how subtle it is that `NodeToMatch`
can be modified by `MSU` here. Maybe passing an `SDNode **` would be
better than an `SDNode *&` so that the caller passes in `&NodeToMatch`
and it's obvious something interesting is happening here?
> MatchScopes));
>
> if (!CheckComplexPattern(NodeToMatch, RecordedNodes[RecNo].second,
> Index: test/CodeGen/X86/dag-update-nodetomatch.ll
> ===================================================================
> --- /dev/null
> +++ test/CodeGen/X86/dag-update-nodetomatch.ll
> @@ -0,0 +1,80 @@
> +; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
> +; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
> +
> +%struct.i = type { i32, i24 }
> +%struct.m = type { %struct.i }
> +
> + at a = local_unnamed_addr global i32 0, align 4
> + at b = local_unnamed_addr global i16 0, align 2
> + at c = local_unnamed_addr global i16 0, align 2
> + at e = local_unnamed_addr global i16 0, align 2
> + at l = local_unnamed_addr global %struct.i zeroinitializer, align 4
> + at k = local_unnamed_addr global %struct.m zeroinitializer, align 4
> +
> +; Function Attrs: norecurse nounwind uwtable
> +define void @_Z1nv() local_unnamed_addr {
> +; CHECK-LABEL: _Z1nv:
> +; CHECK: # BB#0: # %entry
> +; CHECK-NEXT: movl k+{{.*}}(%rip), %ecx
> +; CHECK-NEXT: movswl {{.*}}(%rip), %r9d
> +; CHECK-NEXT: movswl {{.*}}(%rip), %r8d
> +; CHECK-NEXT: movl {{.*}}(%rip), %eax
> +; CHECK-NEXT: movl {{.*}}(%rip), %esi
> +; CHECK-NEXT: movl %esi, %edi
> +; CHECK-NEXT: shll $7, %edi
> +; CHECK-NEXT: sarl $7, %edi
> +; CHECK-NEXT: negl %edi
> +; CHECK-NEXT: testl %eax, %eax
> +; CHECK-NEXT: cmovel %eax, %edi
> +; CHECK-NEXT: movzwl %cx, %r10d
> +; CHECK-NEXT: leal (%r9,%r10,2), %ecx
> +; CHECK-NEXT: addl %r8d, %ecx
> +; CHECK-NEXT: cmpl %edi, %ecx
> +; CHECK-NEXT: sete %dil
> +; CHECK-NEXT: testl $33554431, %esi # imm = 0x1FFFFFF
> +; CHECK-NEXT: sete %dl
> +; CHECK-NEXT: orb %dil, %dl
> +; CHECK-NEXT: movzbl %dl, %edx
> +; CHECK-NEXT: movw %dx, {{.*}}(%rip)
> +; CHECK-NEXT: shrl $31, %ecx
> +; CHECK-NEXT: xorl $1, %ecx
> +; CHECK-NEXT: addl %r10d, %ecx
> +; CHECK-NEXT: # kill: %CL<def> %CL<kill> %ECX<kill>
> +; CHECK-NEXT: sarl %cl, %eax
> +; CHECK-NEXT: movw %ax, {{.*}}(%rip)
> +; CHECK-NEXT: retq
> +entry:
> + %bf.load = load i32, i32* bitcast (i24* getelementptr inbounds (%struct.m, %struct.m* @k, i64 0, i32 0, i32 1) to i32*), align 4
> + %0 = load i16, i16* @c, align 2
> + %conv = sext i16 %0 to i32
> + %1 = load i16, i16* @b, align 2
> + %conv1 = sext i16 %1 to i32
> + %2 = load i32, i32* @a, align 4
> + %tobool = icmp ne i32 %2, 0
> + %bf.load3 = load i32, i32* getelementptr inbounds (%struct.i, %struct.i* @l, i64 0, i32 0), align 4
> + %bf.shl = shl i32 %bf.load3, 7
> + %bf.ashr = ashr exact i32 %bf.shl, 7
> + %bf.clear = shl i32 %bf.load, 1
> + %factor = and i32 %bf.clear, 131070
> + %add13 = add nsw i32 %factor, %conv
> + %add15 = add nsw i32 %add13, %conv1
> + %bf.ashr.op = sub nsw i32 0, %bf.ashr
> + %add28 = select i1 %tobool, i32 %bf.ashr.op, i32 0
> + %tobool29 = icmp eq i32 %add15, %add28
> + %phitmp = icmp eq i32 %bf.ashr, 0
> + %.phitmp = or i1 %phitmp, %tobool29
> + %conv37 = zext i1 %.phitmp to i16
> + store i16 %conv37, i16* @e, align 2
> + %bf.clear39 = and i32 %bf.load, 65535
> + %factor53 = shl nuw nsw i32 %bf.clear39, 1
> + %add46 = add nsw i32 %factor53, %conv
> + %add48 = add nsw i32 %add46, %conv1
> + %add48.lobit = lshr i32 %add48, 31
> + %add48.lobit.not = xor i32 %add48.lobit, 1
> + %add51 = add nuw nsw i32 %add48.lobit.not, %bf.clear39
> + %shr = ashr i32 %2, %add51
> + %conv52 = trunc i32 %shr to i16
> + store i16 %conv52, i16* @b, align 2
> + ret void
> +}
> +
More information about the llvm-commits
mailing list