Page MenuHomePhabricator

[update_cc_test_checks] Support 'clang | opt | FileCheck'
ClosedPublic

Authored by simon_tatham on Oct 3 2019, 9:37 AM.

Details

Summary

Some clang lit tests use a pipeline of the form

// RUN: %clang [args] -O0 %s | opt [specific optimizations] | FileCheck %s

to make the expected test output depend on as few optimization phases
as possible, for stability. But when you write a RUN line of this
form, you lose the ability to use update_cc_test_checks.py to
automatically generate the expected output, because it only supports
two-stage pipelines consisting of '%clang | FileCheck' (or %clang_cc1).

This change extends the set of supported RUN lines so that pipelines
with an invocation of opt in the middle can still be automatically
handled.

To implement it, I've adjusted get_function_body() so that it can
cope with an arbitrary sequence of intermediate pipeline commands. But
the code that decides which RUN lines to consider is more
conservative: it only adds clang | opt | FileCheck to the set of
supported lines, because I didn't want to accidentally include some
other kind of line that doesn't output IR at all.

(Also in this commit is the minimal change to make this script work at
all, after r373912 added an extra parameter to add_ir_checks.)

Diff Detail

Event Timeline

simon_tatham created this revision.Oct 3 2019, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 9:37 AM

For the record, is this intended to be used in upstream clang tests?

There are 30 existing clang CodeGen tests using %clang ... | opt ... | FileCheck, but many (mem2reg) are used to increase readability while some are probably misuses of opt. +@RKSimon who has expressed concerns of maintenance problems on D68230.

+@t.p.northover and @spatel who have added opt to clang tests.

llvm/utils/update_cc_test_checks.py
115

If a RUN line uses opt, you'll need a similar option for opt here.

This is named --opt-binary in update_test_checks.py, but --opt might be more convenient.

@lebedev.ri, yes, I was planning to use it for the tests I'm adding in D67161, after a review comment in there suggested I should be using -O0 | opt -mem2reg in place of -O3.

spatel added a comment.Oct 4 2019, 7:48 AM

+@t.p.northover and @spatel who have added opt to clang tests.

I don't remember that...but I'll trust that's correct over my memory. :)
For reference, we discussed the compromise solution of using "opt -mem2reg" in:
D17999
(and there's a rough script there that might have been used to help implement update_cc_test_checks.py)

Added an option as requested to point at the opt binary. Unlike the
existing --clang, I've made it only lazily give an error: in the
common case where the test file doesn't try to call opt in the first
place, it shouldn't be necessary to have it available.

Also, I've added a fix to the add_ir_checks call, which stopped
working recently due to r373912.

simon_tatham edited the summary of this revision. (Show Details)Oct 9 2019, 6:05 AM
dmgreen added a subscriber: dmgreen.Oct 9 2019, 7:48 AM

This looks very useful to me (I've often wanted something similar with opt -> llc). It would make all the testing MVE intrinsics a lot more readable (to the extent that I'm not sure what we would do without it!)

MaskRay accepted this revision.Oct 10 2019, 12:17 AM
MaskRay added inline comments.
llvm/utils/update_cc_test_checks.py
225

'opt'

This revision is now accepted and ready to land.Oct 10 2019, 12:17 AM
This revision was automatically updated to reflect the committed changes.