Page MenuHomePhabricator

[Bitcode] Update CHECK-DAG usage in tests
ClosedPublic

Authored by jdenny on Jul 1 2019, 2:25 PM.

Details

Summary

This patch adjusts tests not to depend on deprecated FileCheck
behavior that permits overlapping matches within a block of
CHECK-DAG directives:

  1. thinlto-function-summary-originalnames.ll: The directive with the pattern <COMBINED is surely intended to match <COMBINED (note the trailing space), but it instead matches <COMBINED_GLOBALVAR_INIT_REFS, for which there is a separate directive. With the deprecated behavior, both directives match the latter text and neither match the former text. I've adjusted the former directive so it matches only the former text.
  1. thinlto-summary-local-5.0.ll: Two directives have identical patterns when they were clearly meant to have different patterns.
  1. upgrade-pointer-address-space.ll: There are three identical directives but only two occurrences of the matching text. With the deprecated behavior, they always match exactly the same text, so the behavior can't have been useful. I removed one of the directives and converted the other two from CHECK-DAG to CHECK.

While I have attempted to preserve the original intent of these tests,
I might have misunderstood something, so please point out any
problems. Your explanation might prove helpful as I address similar
issues in other tests.

Diff Detail

Repository
rL LLVM

Event Timeline

jdenny created this revision.Jul 1 2019, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 2:25 PM
jdenny added inline comments.Jul 1 2019, 4:40 PM
llvm/test/Bitcode/upgrade-pointer-address-space.ll
3 ↗(On Diff #207409)

Ugh, sorry, there are two occurrences of the matching text, so we can have two CHECK directives. CHECK-DAG does not appear useful. Will fix.

+adrian who reviewed the patch that introduced upgrade-pointer-address-space.ll.

llvm/test/Bitcode/upgrade-pointer-address-space.ll
3 ↗(On Diff #207409)

I went back to the review that introduced this test (D29670) and I can't tell what is being tested here. It looks like prior LLVM versions would have passed this test as well, so it is not exercising the patch at all.
@kzhuravl what was the intent of this test? Proving that the disassembly reports two arbitrary pointer types with unspecified characteristics does not seem very useful.

jdenny updated this revision to Diff 207557.Jul 2 2019, 7:09 AM
jdenny edited the summary of this revision. (Show Details)

Fix minor problems in the summary and a test.

@probinson : Thanks for the review. I wanted to get those tweaks out of my hands, but I'm happy to adjust further once we understand the original intention of that test.

jdenny marked an inline comment as done.Jul 2 2019, 7:10 AM
aprantl added inline comments.Jul 2 2019, 9:44 AM
llvm/test/Bitcode/upgrade-pointer-address-space.ll
3 ↗(On Diff #207409)

IIUC, this is testing that the older bitcode format, without the address space field introduced in D29670, can be upgraded correctly.

probinson accepted this revision.Jul 2 2019, 1:29 PM

LGTM

llvm/test/Bitcode/upgrade-pointer-address-space.ll
3 ↗(On Diff #207409)

Thanks. I see older releases are also producing two pointer types so I guess that's sufficient, and Joel's conversion to two non-DAG CHECKs is correct.

This revision is now accepted and ready to land.Jul 2 2019, 1:29 PM
This revision was automatically updated to reflect the committed changes.