This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] NFC, update Switch tests to HEAD so I can see if my changes change anything
AbandonedPublic

Authored by shawnl on Apr 25 2019, 1:56 PM.

Diff Detail

Event Timeline

shawnl created this revision.Apr 25 2019, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2019, 1:56 PM
shawnl edited the summary of this revision. (Show Details)Apr 25 2019, 1:56 PM
shawnl updated this revision to Diff 196734.Apr 25 2019, 3:18 PM
shawnl edited the summary of this revision. (Show Details)

include more tests

shawnl planned changes to this revision.Apr 25 2019, 3:22 PM
shawnl requested review of this revision.Apr 25 2019, 4:06 PM
shawnl edited the summary of this revision. (Show Details)

I was confused because the disable-lookup-table.ll not having a datalayout was causing problems.

nikic added a subscriber: nikic.Apr 26 2019, 12:23 AM
nikic added inline comments.
test/Transforms/SimplifyCFG/X86/switch-covered-bug.ll
11 ↗(On Diff #196734)

Here and in the next file: You've generated new CHECK lines with update_test_checks, but the old ones are still there. These need to be manually dropped.

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

Why the addition of -O3 here? Generally tests in test/SimplifyCFG should use only -simplifycfg.

jmolloy accepted this revision.Apr 26 2019, 12:36 AM

LGTM.

test/Transforms/SimplifyCFG/X86/lit.local.cfg
1 ↗(On Diff #196734)

Revert hunk in this file.

This revision is now accepted and ready to land.Apr 26 2019, 12:36 AM
shawnl planned changes to this revision.Apr 26 2019, 8:13 AM

I will look into the issues nikic brought up

shawnl updated this revision to Diff 197004.Apr 28 2019, 1:20 AM
This revision is now accepted and ready to land.Apr 28 2019, 1:20 AM
nikic added inline comments.Apr 28 2019, 2:13 AM
test/Transforms/SimplifyCFG/X86/lit.local.cfg
1 ↗(On Diff #196734)

This one is not done yet.

test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll
375 ↗(On Diff #197004)

A leftover CHECK.

1589 ↗(On Diff #197004)

Leftover CHECK.

1676 ↗(On Diff #197004)

Leftover CHECKs.

test/Transforms/SimplifyCFG/switch_create-custom-dl.ll
58

It looks like these check lines were added with an incorrect prefix originally and didn't do anything... They can be dropped now that the proper CHECKs are there.

89

Same here.

752

Leftover CHECKs.

816

Leftover CHECKS.

848

Leftover CHECKS.

test/Transforms/SimplifyCFG/switch_create.ll
742

Leftover CHECKS. (Also in the rest of this file.)

nikic requested changes to this revision.May 10 2019, 1:22 PM

Still needs some duplicates CHECKs removed, per comments above.

This revision now requires changes to proceed.May 10 2019, 1:22 PM
shawnl updated this revision to Diff 199186.May 12 2019, 2:33 PM

review. Also update a few more tests.

nikic requested changes to this revision.May 13 2019, 12:56 PM

A few last issues...

test/Transforms/SimplifyCFG/switch-dead-default.ll
246–247

This seems odd. Are you sure these are the results without any other patches?

test/Transforms/SimplifyCFG/switch-on-const-select.ll
158

Does this test actually pass after being regenerated? I'd expect an error due to the undefined variable here. This probably needs to be replaced with just #0 if you're using it above.

test/Transforms/SimplifyCFG/switch_create-custom-dl.ll
787

More leftovers.

test/Transforms/SimplifyCFG/switch_create.ll
739

A few more left here...

This revision now requires changes to proceed.May 13 2019, 12:56 PM
shawnl planned changes to this revision.May 13 2019, 4:16 PM
shawnl marked an inline comment as done.

I'll get rid of the leftovers.

test/Transforms/SimplifyCFG/switch-on-const-select.ll
158

You are referring to the other file, but yes it fails, but no, this is not a bug in LLVM, but rather a bug in the test.

shawnl marked an inline comment as done.May 13 2019, 4:17 PM

comment didn't get included.

test/Transforms/SimplifyCFG/switch-dead-default.ll
246–247

Yes, and it is correct. I opened a bug and then was corrected that this is correct. Here is the patch removing this buggy test. https://reviews.llvm.org/D60859

shawnl updated this revision to Diff 199350.May 13 2019, 4:58 PM
nikic added inline comments.May 14 2019, 12:33 AM
test/Transforms/SimplifyCFG/switch-on-const-select.ll
158

I'm referring to the use of [[$NUW]] here, even though the corresponding [[$NUW:#[0-9]+]] above has been removed.

shawnl updated this revision to Diff 200867.May 22 2019, 8:34 PM

remove line nikic mentioned

shawnl abandoned this revision.May 26 2019, 7:45 AM

applied 361725