Page MenuHomePhabricator

[utils] Add utils/update_cc_test_checks.py
ClosedPublic

Authored by MaskRay on Jan 30 2018, 3:07 PM.

Details

Summary

A utility to update LLVM IR in C/C++ FileCheck test files.

Example RUN lines in .c/.cc test files:

// RUN: %clang -S -Os -DXX %s -o - | FileCheck %s
// RUN: %clangxx -S -Os %s -o - | FileCheck -check-prefix=IR %s

Usage:

% utils/update_cc_test_checks.py --llvm-bin=release/bin test/a.cc
% utils/update_cc_test_checks.py --c-index-test=release/bin/c-index-test --clang=release/bin/clang /tmp/c/a.cc

Example input:

// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
// RUN: %clang -emit-llvm -S -Os -DXX %s -o - | FileCheck -check-prefix=AA %s
// RUN: %clangxx -emit-llvm -S -Os %s -o - | FileCheck -check-prefix=BB %s
using T =
#ifdef XX
    int __attribute__((vector_size(16)))
#else
    short __attribute__((vector_size(16)))
#endif
    ;

T foo(T a) {
  return a + a;
}

After utils/update_cc_test_checks.py --llvm-bin=release/bin a.cc

// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
// RUN: %clang -emit-llvm -S -Os -DXX %s -o - | FileCheck -check-prefix=AA %s
// RUN: %clangxx -emit-llvm -S -Os %s -o - | FileCheck -check-prefix=BB %s
using T =
#ifdef XX
    int __attribute__((vector_size(16)))
#else
    short __attribute__((vector_size(16)))
#endif
    ;

// AA-LABEL: _Z3fooDv4_i:
// AA:       entry:
// AA-NEXT:    %add = shl <4 x i32> %a, <i32 1, i32 1, i32 1, i32 1>
// AA-NEXT:    ret <4 x i32> %add
//
// BB-LABEL: _Z3fooDv8_s:
// BB:       entry:
// BB-NEXT:    %add = shl <8 x i16> %a, <i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1>
// BB-NEXT:    ret <8 x i16> %add
T foo(T a) {
  return a + a;
}

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Remove unused add_checks

spatel edited reviewers, added: asb, craig.topper, RKSimon; removed: spatel.Jan 31 2018, 6:43 AM

Adding others who might be interested in this functionality, and must be more qualified to review python changes (since I don't know it at all).

We may be creating a moral hazard by making it easy to check asm from c/c++ (we're generally not supposed to have end-to-end unit/regression tests), but it's nice to have that functionality. :)

spatel added a subscriber: spatel.Jan 31 2018, 6:44 AM

Ideally I think the refactoring for update_llc_test_checks.py/update_test_checks.py should be in a seperate patch that lands first and a patch adding update_cc_test_checks.py is added dependening on it.

asb added a comment.Feb 1 2018, 6:19 AM

I agree with @RKSimon that splitting the refactoring to a separate patch would make sense. I've previously missed having an update_cc_test_checks.py for IR tests, so think this is a great improvement. End-to-end clang->assembly tests may not be welcome upstream, but given they're trivial to support after this refactoring it would seem mean not to expose that functionality to out-of-tree users who want it.

@RKSimon @asb Thanks for review. I have git rm utils/update_cc_test_checks.py and moved the refactoring part to https://reviews.llvm.org/D42805

MaskRay updated this revision to Diff 132679.Feb 2 2018, 2:02 PM

Ignore '//\n' lines

MaskRay edited the summary of this revision. (Show Details)Feb 7 2018, 11:11 AM
MaskRay updated this revision to Diff 133302.Feb 7 2018, 2:10 PM

Add --clang-args for debugging

MaskRay updated this revision to Diff 133712.Feb 9 2018, 4:08 PM

Forward clang command line options to c-index-test (e.g. -std=c++11 -D which may change the AST)

Thanks everyone for review! https://reviews.llvm.org/D42805 has landed (refactoring utils/update* scripts so that this new script can re-use the CHECK line parsing and adding functionality).
should we push forward this one?

c-index-test is awesome. Different name mangling is now supported:

// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
// RUN: %clang -emit-llvm -S -Os -DXX %s -o - | FileCheck %s
// RUN: %clang -emit-llvm -S -Os %s -o - | FileCheck %s
struct A {};
struct B {};

using T =
#ifdef XX
A
#else
B
#endif
;

// CHECK-LABEL: _Z3foo1A:
// CHECK:       entry:
// CHECK-NEXT:    ret void
// CHECK-LABEL: _Z3foo1B:
// CHECK:       entry:
// CHECK-NEXT:    ret void
T foo(T x) {
  return x;
}

OT: It seems I can use arc diff refactor-update_test_checks to make this revision dependent on another git branch (refactor-update_test_checks).

MaskRay updated this revision to Diff 133916.Feb 12 2018, 12:29 PM

Remove --c-index-test-args and use --clang-args for c-index-test

MaskRay updated this revision to Diff 133964.Feb 12 2018, 5:10 PM

Support different mangled names on the same line

MaskRay updated this revision to Diff 133969.Feb 12 2018, 5:50 PM

Support %clangxx

Leaving it as an IR only check seems like the right thing to do. We definitely don't want to encourage people to have .cc -> .s checks.

utils/update_cc_test_checks.py
68 ↗(On Diff #134248)

Perhaps you could split this into some functions?

MaskRay updated this revision to Diff 134698.Feb 16 2018, 12:49 PM

Add --wont-emit-asm-in-clang-testsuite notice to discourage asm tests

MaskRay marked an inline comment as done.Feb 16 2018, 12:51 PM
MaskRay added inline comments.
utils/update_cc_test_checks.py
68 ↗(On Diff #134248)
% utils/update_cc_test_checks.py --llvm-bin ~/Dev/llvm/release/bin /tmp/d/a.cc                                   
Enable --wont-emit-asm-in-clang-testsuite to acknowledge that asm tests are discouraged in Clang testsuite, and you won't use this script to check in them.

I was inspired by GNU parallel and added this option --wont-emit-asm-in-clang-testsuite

MaskRay updated this revision to Diff 134705.Feb 16 2018, 1:06 PM
MaskRay marked an inline comment as done.

Change the notice. Time when I wish I was a lawyer.

MaskRay updated this revision to Diff 135706.Feb 23 2018, 2:48 PM

Remove asm tests

MaskRay edited the summary of this revision. (Show Details)Feb 23 2018, 2:55 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay edited the summary of this revision. (Show Details)

Start of some comment requests.

utils/UpdateTestChecks/asm.py
97 ↗(On Diff #135710)

Hmm?

utils/UpdateTestChecks/common.py
32 ↗(On Diff #135710)

Can you comment this change?

utils/update_cc_test_checks.py
189 ↗(On Diff #135710)

Update comment.

MaskRay updated this revision to Diff 135754.Feb 23 2018, 6:05 PM
MaskRay marked 3 inline comments as done.

Update comments

MaskRay marked an inline comment as done.Feb 23 2018, 6:05 PM
MaskRay added inline comments.
utils/UpdateTestChecks/asm.py
97 ↗(On Diff #135710)

So that it will not warn that args (parsed command line options) does not have the attribute.

Thanks for working on this!

It almost worked for me but it generated the following LABEL assertions for me
// CHECK-LABEL: test: which I then had to manually change to // CHECK-LABEL: define i32 @test(

Thanks for working on this!

It almost worked for me but it generated the following LABEL assertions for me
// CHECK-LABEL: test: which I then had to manually change to // CHECK-LABEL: define i32 @test(

Thanks. The label is not kept as is because it has been scrubbed in the same way as tests are scrubbed by utils/update_test_checks.py

% utils/update_test_checks.py --opt-binary=~/Dev/llvm/release/bin/opt test/CodeGen/Generic/expand-experimental-reductions.ll
...............................
define i64 @or_i64(<2 x i64> %vec) {
; CHECK-LABEL: @or_i64(
; CHECK-NEXT:  entry:
; CHECK-NEXT:    [[RDX_SHUF:%.*]] = shufflevector <2 x i64> [[VEC:%.*]], <2 x i64> undef, <2 x i32> <i32 1, i32 undef>
; CHECK-NEXT:    [[BIN_RDX:%.*]] = or <2 x i64> [[VEC]], [[RDX_SHUF]]
; CHECK-NEXT:    [[TMP0:%.*]] = extractelement <2 x i64> [[BIN_RDX]], i32 0
; CHECK-NEXT:    ret i64 [[TMP0]]
;
echristo accepted this revision.Mar 1 2018, 10:52 PM
This revision is now accepted and ready to land.Mar 1 2018, 10:52 PM
arichardson requested changes to this revision.Mar 2 2018, 2:52 AM

If I have the following:

// RUN: %clang_cc1 -emit-llvm -o - %s -O2 | FileCheck %s

int test(int a, int b) {
  return a + b;
}

After running update_cc1_checks.py I get the following test (which will not pass because of the broken label):

// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
// RUN: %clang_cc1 -emit-llvm -o - %s -O2 | FileCheck %s

// CHECK-LABEL: tests:
// CHECK:       entry:
// CHECK-NEXT:    %add = add nsw i32 %b, %a
// CHECK-NEXT:    ret i32 %add
int tests(int a, int b) {
  return a + b;
}

The label assertions doesn't work:

/Users/alex/devel/llvm/tools/clang/test/CodeGen/avr/test.c:7:17: error: expected string not found in input
// CHECK-LABEL: tests:
                ^
<stdin>:1:1: note: scanning from here
; ModuleID = '/Users/alex/devel/llvm/tools/clang/test/CodeGen/avr/test.c'
^
<stdin>:7:13: note: possible intended match here
define i32 @tests(i32 signext %a, i32 signext %b) local_unnamed_addr #0 {
utils/update_cc_test_checks.py
50 ↗(On Diff #135754)

I noticed this does not work on MacOS because the mangled_name will be _test on OSX but the IR still uses @test, so the script does not add any IR checks

60 ↗(On Diff #135754)

If I add the following lines here the script works on MacOS:

if mangled == "_" + spell:
  # HACK for MacOS (where the mangled name includes an _ for C but the IR won't):
  mangled = spell
This revision now requires changes to proceed.Mar 2 2018, 2:52 AM
MaskRay updated this revision to Diff 136776.Mar 2 2018, 9:38 AM

Add arichardson's hack for Mac OS

MaskRay marked 2 inline comments as done.Mar 2 2018, 9:39 AM
MaskRay added inline comments.
utils/update_cc_test_checks.py
60 ↗(On Diff #135754)

Thanks! Applied the hack

This revision was not accepted when it landed; it landed in state Needs Review.Mar 2 2018, 9:39 AM
Closed by commit rL326591: [utils] Add utils/update_cc_test_checks.py (authored by MaskRay, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.

LGTM, I guess the CHECK-LABEL thing can be fixed later.

LGTM, I guess the CHECK-LABEL thing can be fixed later.

Sorry for committing without the revision being in the "approved" status , I did not know I should press "Add Action..." to address your comment and wait for you to remove the "Request Changes" status (the cross icon). Would you like me to revert this revision or send another revision after to fix this? I shall learn more about update_test_checks.py update_llc_test_checks.py

No need to revert, I think it is fine to improve the script in follow-up revisions. Thanks again for working on this!

Created https://reviews.llvm.org/D44400

I had both asm and IR check lines in mind but broke it after making it IR only.

utils/UpdateTestChecks/asm.py:add_asm_checks used by utils/update_llc_test_checks.py, has some duplication and someone should fix it.