This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] remove bogus test case; NFC
ClosedPublic

Authored by shawnl on Apr 18 2019, 2:40 AM.

Download Raw Diff

Details

Reviewers

RKSimon
nikic
reames

Commits

rG7f22fecac29e: [SimplifyCFG] NFC; remove bogus test case
rL362499: [SimplifyCFG] NFC; remove bogus test case

Summary

Even if one bit is defined, the code is not clear what it is suppose to do.

See 41530

Diff Detail

Event Timeline

shawnl created this revision.Apr 18 2019, 2:40 AM

Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2019, 2:40 AM

Herald added a subscriber: llvm-commits. · View Herald Transcript

Patch doesn't seem to be against master, here's how the test looks right now: https://github.com/llvm-mirror/llvm/blob/master/test/Transforms/SimplifyCFG/switch-dead-default.ll#L160

In any case, agree that the test doesn't make sense and we should drop it. The test wants to assert that some bits are undef, but that's not what the IR does and I don't think it's even possible to do that in any meaningful way. It was added in D12497, so @reames might want to double check.

@reames Do you have any objection to removing this test?

@reames Any final comments?

In D60859#1526786, @RKSimon wrote:

@reames Any final comments?

None, feel free to kill it.

In D60859#1528116, @reames wrote:

In D60859#1526786, @RKSimon wrote:

@reames Any final comments?

None, feel free to kill it.

Thanks - LGTM

This revision is now accepted and ready to land.Jun 4 2019, 1:16 AM

Closed by commit rL362499: [SimplifyCFG] NFC; remove bogus test case (authored by shawnl). · Explain WhyJun 4 2019, 7:17 AM

This revision was automatically updated to reflect the committed changes.

Most bots on the waterfall are unhappy after this, e.g. http://lab.llvm.org:8011/builders/clang-atom-d525-fedora-rel/builds/25030/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Aswitch-dead-default.ll -- can you fix quickly, or if it takes a while revert?

@thakis Looks like it got fixed in rL362501.

Revision Contents

Path

Size

test/

Transforms/

SimplifyCFG/

switch-dead-default.ll

25 lines

Diff 195699

test/Transforms/SimplifyCFG/switch-dead-default.ll

Context not available.
	default:	default:
	call void @foo(i32 2)	call void @foo(i32 2)
	ret void	ret void
	}	}

	;; All but one bit known undef
	;; Note: This is currently testing an optimization which doesn't trigger. The
	;; case this is protecting against is that a bit could be assumed both zero
	;; or one given we know it's undef. ValueTracking doesn't do this today,
	;; but it doesn't hurt to confirm.
	define void @test8(i8 %a) {
	; CHECK-LABEL: @test8(
	; CHECK-NEXT: unreachable
	;
	%and = and i8 %a, 254
	%cmp = icmp eq i8 %and, undef
	call void @llvm.assume(i1 %cmp)
	switch i8 %a, label %default [i8 255, label %true
	i8 254, label %false]
	true:
	call void @foo(i32 1)
	ret void
	false:
	call void @foo(i32 3)
	ret void
	default:
	call void @foo(i32 2)
	ret void
	}

	declare void @llvm.assume(i1)	declare void @llvm.assume(i1)
Context not available.