This is an archive of the discontinued LLVM Phabricator instance.

[Target/X86] Fix crash in assignTopologicalOrder()
ClosedPublic

Authored by davide on May 9 2015, 2:26 PM.

Download Raw Diff

Details

Reviewers

davide
bkramer

Summary

Disclaimer: I'm not really familiar with the lowering code, and it looks pretty scary. If this doesn't make sense, I apologize in advance.

It seems that call to ReplaceAllUsesWith() when we're turning SHL -> AND is not really required and causes problems with the testcase below. It ends up producing an incorrect DAG, with a cycle in, so a topological order can't be assigned.

In particular this issue shows up if getConstant() gets an existing node in the graph rather than generating a new one, which is then incorrectly deleted causing the issue.

As an added note, In the test, {2, 127} can be changed to {4, 63} or {8, 31} and the issue remain.

%mul = mul i8 %trunc, 2
%and = and i8 %trunc, 127

Diff Detail

Event Timeline

davide updated this revision to Diff 25421.May 9 2015, 2:26 PM

davide retitled this revision from to [Target/X86] Fix crash in assignTopologicalOrder().

davide updated this object.

davide edited the test plan for this revision. (Show Details)

davide added a reviewer: bkramer.

davide set the repository for this revision to rL LLVM.

davide added a subscriber: Unknown Object (MLST).

This isn't the right fix. The resulting code from the test case now looks like this

movb	a(%rip), %cl
andb	$127, %cl
setne	%al
testb	%cl, %cl
setne	b(%rip)

There should only be one testb, no andb. I think we just have to remove the dead node after replacing all nodes, I'll commit a fix for that.

r237828

This revision is now accepted and ready to land.May 20 2015, 1:17 PM

davide closed this revision.May 20 2015, 1:17 PM

Revision Contents

Path

Size

lib/

Target/

X86/

X86ISelLowering.cpp

1 line

test/

CodeGen/

X86/

icmp-lowering-crash.ll

19 lines

Diff 25421

lib/Target/X86/X86ISelLowering.cpp

	Show First 20 Lines • Show All 592 Lines • ▼ Show 20 Lines
	break;			break;
	APInt Mask = ArithOp.getOpcode() == ISD::SRL			APInt Mask = ArithOp.getOpcode() == ISD::SRL
	? APInt::getHighBitsSet(BitWidth, BitWidth - ShAmt)			? APInt::getHighBitsSet(BitWidth, BitWidth - ShAmt)
	: APInt::getLowBitsSet(BitWidth, BitWidth - ShAmt);			: APInt::getLowBitsSet(BitWidth, BitWidth - ShAmt);
	if (!Mask.isSignedIntN(32)) // Avoid large immediates.			if (!Mask.isSignedIntN(32)) // Avoid large immediates.
	break;			break;
	SDValue New = DAG.getNode(ISD::AND, dl, VT, Op->getOperand(0),			SDValue New = DAG.getNode(ISD::AND, dl, VT, Op->getOperand(0),
	DAG.getConstant(Mask, dl, VT));			DAG.getConstant(Mask, dl, VT));
	DAG.ReplaceAllUsesWith(Op, New);
	Op = New;			Op = New;
	}			}
	break;			break;

	case ISD::AND:			case ISD::AND:
	// If the primary and result isn't used, don't bother using X86ISD::AND,			// If the primary and result isn't used, don't bother using X86ISD::AND,
	// because a TEST instruction will be better.			// because a TEST instruction will be better.
	if (!hasNonFlagsUse(Op))			if (!hasNonFlagsUse(Op))
	▲ Show 20 Lines • Show All 592 Lines • Show Last 20 Lines

test/CodeGen/X86/icmp-lowering-crash.ll

				; RUN: llc < %s > /dev/null
				target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
				target triple = "x86_64-unknown-linux-gnu"

				@a = external global i32, align 4
				@b = external global i1, align 4

				define i1 @f() {
				entry:
				%load = load i32, i32* @a, align 4
				%trunc = trunc i32 %load to i8
				%mul = mul i8 %trunc, 2
				%icmp1 = icmp ne i8 %mul, 0
				store i1 %icmp1, i1* @b, align 4
				%and = and i8 %trunc, 127
				%icmp2 = icmp ne i8 %and, 0
				ret i1 %icmp2
				}