Page MenuHomePhabricator

[Utils] Deal with occasionally deleted functions
ClosedPublic

Authored by jdoerfert on Oct 10 2019, 6:26 PM.

Details

Summary

When functions exist for some but not all run lines we need to be
careful when selecting the prefix. So far, a common prefix was
potentially chosen as there was never a "conflict" that would have
caused otherwise. With this patch we avoid common prefixes if they
are used by run lines that do not emit the function.

Tested as part of D68766 and the follow up that adds the Attributor test
lines to all argument promotion tests.

Diff Detail

Event Timeline

jdoerfert created this revision.Oct 10 2019, 6:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 6:26 PM
Herald added a subscriber: bollu. · View Herald Transcript
jdoerfert added a comment.EditedOct 28 2019, 2:50 PM

The difference is when we run update_test_checks.py on the following code

; RUN: opt -S < %s | FileCheck %s --check-prefixes=ALL,FIRST
; RUN: opt -S -globalopt < %s | FileCheck %s --check-prefixes=ALL,SECOND

define internal void @f() {
  ret void
}

we will see

; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S < %s | FileCheck %s --check-prefixes=ALL,FIRST
; RUN: opt -S -globalopt < %s | FileCheck %s --check-prefixes=ALL,SECOND

define internal void @f() {
; FIRST-LABEL: define {{[^@]+}}@f(
; FIRST-NEXT:    ret void
;
  ret void
}

and not

; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S < %s | FileCheck %s --check-prefixes=ALL,FIRST
; RUN: opt -S -globalopt < %s | FileCheck %s --check-prefixes=ALL,SECOND

define internal void @f() {
; ALL-LABEL: @f(
; ALL-NEXT:    ret void
;
  ret void
}

ping

xbolva00 resigned from this revision.Nov 30 2019, 5:55 AM
lebedev.ri requested changes to this revision.Dec 6 2019, 6:05 AM

Now that i believe there's some testing infra for these utils, add/update tests for this?

This revision now requires changes to proceed.Dec 6 2019, 6:05 AM
jdoerfert updated this revision to Diff 235667.Mon, Dec 30, 7:27 PM

Add a regression test

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

lebedev.ri added inline comments.Mon, Dec 30, 11:50 PM
llvm/utils/UpdateTestChecks/common.py
269

I'm having a hard time figuring out whether this blacklist is per-function or global.
I.e. what happens if sometimes_deleted_function.ll has a second function,
that *does* exist for all check prefixes. Will it also only be checked in FIRST check prefix?

Make the test more expressive

jdoerfert marked 2 inline comments as done.Mon, Dec 30, 11:55 PM
jdoerfert added inline comments.
llvm/utils/UpdateTestChecks/common.py
269

It's a *per function* blacklist. I updated the test to show that ALL is used for a function that is always present.

lebedev.ri accepted this revision.Tue, Dec 31, 12:11 AM

Really convoluted, but seems to be okay.

llvm/utils/UpdateTestChecks/common.py
269

Name is too generic, should maybe be prefix_blacklist.

This revision is now accepted and ready to land.Tue, Dec 31, 12:11 AM

Unit tests: fail. 61156 tests passed, 1 failed and 728 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_sharedtimedmutex_requirements/thread_sharedtimedmutex_class/try_lock.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This revision was automatically updated to reflect the committed changes.
jdoerfert marked an inline comment as done.