LGTM

• I would be slightly hesitant here, as it complicates code and none of the public targets needs this. We don't even try the "uint64_t" instead of "unsigned" step but instead just make it all complicated right away...
• I would not commit this without careful measurements to ensure that compilers indeed optimize all of this away and we don't have a compiletime impact.
• You probably can use some of the APInt::tcXXX functions to avoid code duplication.

Finally got around pushing this. You may consider asking for commit access (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access) if you plan further changes.

Testcase? It seems that your change wants to allow the following:

LGTM

Seems fine to me, but needs more documentation and this touching a core regalloc datastructure I'm including Quentin in the reviewers.

I'm confused by this change (and the testcase doesn't really help understanding what is going on). You are probably onto a real bug here where we cannot assume the same lanemasks work for the input/output register class of a copy. However that should be independent of the fact that a full or partial copy is present.

I'm not sure I like this:

• It introduces a new word "cluster", so we end up having the words "MacroFusion" and "Cluster" around which appear to mean the same thing?
• Using computeOperandLatency() to compute something called cluster feels out of place, similar with getReadAdvanceCycles

LGTM. I wish we wouldn't use std::functions here at all and instead something like a base class with two subclasses, but that is for another patch.

LGTM, style suggestions below.

This is out of date. Nowadays targets can decide themselfes whether they want post-ra fusion by overriding TaretPassConfig::createPostMachineScheduler() and adding a dag mutation there.

This seems to have landed a while ago in r298648

In D35422#813830, @fjricci wrote:

Reverted as rL308394 and rL308395

Various greendragon builds started to timeout when running unittests after this commit. Any idea what causes this? Should we revert the commit to bring the builders back?

• Add unittests
• Add documentation

LGTM

I think this broke the greendragon libfuzzer build: http://lab.llvm.org:8080/green/job/libFuzzer/5100/ this one seems like the most likely cause.
Any idea how to get this back to green?

LGTM

In D35287#807305, @efriedma wrote:

Having a default architecture is convenient for messing around... if we're going to do something here, I would rather just kill off "-march".

LGTM

Yep, that was an oversight. Thanks for fixing!

Can you give an example of a typical use? Is this just a different way to express macrofusion opportunities or is this more or less powerfull?

LGTM

Hi, this change leads to llvm-test-suite tramp3d-v4 failing (at least on macOS systems).

Looks good to me. Possible improvement below.

Please, ignore the first part in my last reply that proposes immediately throwing an exception. I realized that we cannot compile that line while -fno-exceptions is enabled. Hence I proposed a different solution that could go as part of llvm::report_bad_alloc_error() (as I proposed later in the review).

So how about

#include <new>
...
x = malloc(...);
if (x == nullptr)
  throw std::bad_alloc();

There is already precedent with
TargetInstrInfo::getSerializableTargetIndices(), getSerializableDirectMachineOperandTargetFlags() and getSerializableBitmaskMachineOperandTargetFlags()

This adds bundle-specific behaviour into the register allocator. I think this is wrong!

In general: This may warrant an RFC to llvm-dev to discuss the scope of these patches. I feel that making all of LLVM abort gracefully in out of memory situations will be hard/unmaintainable. I believe the trick would be to limit the scope. Remembering your EuroLLVM talk it seemed that you would only need IR construction in the long running server process so you can hand over that IR to shorter lived/restartable worker processes doing the actual code generation. So the scope may be limited to this use case avoiding the need to fix all the transformation passes to recover gracefully.

In D34910#797530, @silvas wrote:

Another possible naming convention that might avoid the scenario Matthias is worried about is all/everything like clang's -W options.

In D34910#797469, @mehdi_amini wrote:

In D34910#797402, @silvas wrote:

IIRC the intention was that in-tree work should not be burdened by experimental targets, up to and including breaking them. E.g. an in-tree API migration wouldn't have to update experimental targets. So in that sense LLVM_TARGETS_TO_BUILD=all is working as intended I think.

Yes this is correct: but what matters on this aspect is the *default* isn't it? A bot does not have to specify LLVM_TARGETS_TO_BUILD, and as long as the default does not include the experimental target that seems fine to me.

Changing the meaning of "all" to really mean all targets makes sense. But there is of course a fundamental discussion hidding here:

In D34897#797097, @arsenm wrote:

In D34897#797095, @MatzeB wrote:

Some thoughts:

• Is this motivated by AMDGPU having a huge number of registers in the mask?

Yes. It more than fills my fullscreen terminal window on any call. It wouldn't be so bad if it printed just regunits, but this also prints all of the register tuple combinations as well.

In D34897#797097, @arsenm wrote:

In D34897#797095, @MatzeB wrote:

Some thoughts:

• Is this motivated by AMDGPU having a huge number of registers in the mask?

Yes. It more than fills my fullscreen terminal window on any call. It wouldn't be so bad if it printed just regunits, but this also prints all of the register tuple combinations as well.

• For X86, AArch64, ARM the output certainly takes up a bunch of space but I find the information important enough that I'd like to see it by default
• How about a compromise: We stop using an On/Off switch but instead influence the number of registers printed in the regmask instead of the hardcoded 10 at the moment. Then choose a big default value that easily fits X86, AArch64, ARM etc.

The MIR printer comes up with a symbolic name for the regmask. I was wondering why the debug dump doesn't use that

Some thoughts:

• Is this motivated by AMDGPU having a huge number of registers in the mask?
• For X86, AArch64, ARM the output certainly takes up a bunch of space but I find the information important enough that I'd like to see it by default
• How about a compromise: We stop using an On/Off switch but instead influence the number of registers printed in the regmask instead of the hardcoded 10 at the moment. Then choose a big default value that easily fits X86, AArch64, ARM etc.

See comments below but nothing blocking. LGTM.

In D34830#795804, @dlj wrote:

In D34830#795765, @MatzeB wrote:

What happens when the user is using the lit from "pip install lit" instead of the one coming with llvm?

That should be fine. ShTest has the same semantics before and after, so importing the old or the new version of ShTest will work.

Now, if they were using an old version of test-suite and the new version of lit, the answer would be different.

Thanks for the review!

What happens when the user is using the lit from "pip install lit" instead of the one coming with llvm? Alternative question: Who is able to update the pypy package?

In D34584#794921, @kristof.beyls wrote:

In D34584#794519, @MatzeB wrote:

Before I dive in the meta discussion, are there any issues with the patch itself?

I'm not up to speed on many parts of LNT that this code touches, but reading through it, I only added some optional nitpicks, apart from 1 issue that I think needs to be addressed before committing:
The patch as is will drop documentation on what field names to use in the json representation, making it impossible for people to write a converter from data from other testing/benchmarking systems to the json-format to import into LNT, without going through a lot of effort trying to reverse engineer the format from source code. I think the documentation of the fields with meaning for "nts" must be retained.
My experience is that most of such users of LNT don't grasp the concept of multiple "test-suite formats" such as "nts" or "compile" in the LNT DB, which is why I didn't bother trying to explain that well in the current documentation.
As you suggest later, maybe the json format should become self-describing (I'm maybe pushing your suggestion further than you intended here), but until that happens, I think the documentation on the nts field names that matter should stay.

Once that documentation issue is addressed, I have no objections for this patch to be committed.

In D34584#793766, @kristof.beyls wrote:

In D34584#792859, @MatzeB wrote:

Drop Run/Info/tag: json itself is a schemaless format so it feels more natural without one. The LNT testsuite type is now determined by the submit URL used, or the '-s' commandline flag on the commandline. Both default to 'nts' if omitted.

I don't fully understand how LNT reacts to this, but on the surface makes sense to me. If someone ever had time to do a major redesign of the database schema, there is a chance we might also be able to drop the need for differences in the database schema per test suite, like "nts"? Anyway, that is definitely far out of scope for this patch.

The patch works fine. People using non-nts testsuites (which I have a strong suspicion only exist within apple), may need to supply a -s MySchema flag for the commandline tools or use db_default/v4/MySchema/submitRun instead of db_default/submitRun to express they don't use the 'nts' default.

This is probably not the right patch to discuss LNT database design. But some points anyway:

• IMO making SQL tables too generic (like having one row with MetricType, Test, Run and Value; so we could create new MetricTypes on demand) is a sure way to kill performance. As long as we use SQL we should have 1 table column for each metric.
• A schema is nice when you need to decide bits like the unit of a metric, whether bigger is better or getting a human readable metric name for the website. However we could make good arguments that we only need that on the presentation/analysis side and not as part of the data storage model.

Agreed that not losing performance will be the key challenge for any redesign of the SQL schema to avoid having test-suite-specific tables. Anyway, it's indeed a discussion for another time...

Again unrelated to this patch,

Note that LNT is already designed to be extensible. Schemas like 'nts' or 'compile' switch the metrics available or what fields are considered for identifying/ordering runs. It just fails being user friendly so that people would actually create new schemes or modifying existing when they setup new benchmarks. IMO the current database design is good enough and we rather need a patch that makes creating and installing new schemes easy. I imagine a world where testsuite come with lnt.schema.json which describes the names/bigger is better/etc. for a testsuite and then when you setup a server you just reference those schemas in your lnt.cfg and get a server that matches your testsuite.

Right, seems like a reasonable way forward to me. I'm wondering if adding extra fields to a test-suite becoming as simple as just describing the extra fields in the json file you import, rather than at server configuration time, would even be nicer? Anyway - a discussion for the future.

Previously:

"Machine": {

"name": "mymachine",
"Info": {
  "hardware": "HAL 9000"
}

},
"Run": {

"Start Time": "2017-05-16 21:31:42",
"End Time": "2017-05-16 21:31:42",
"Info": {
  "__report_version__": "1",

I don't know what report_version is used for today, but would keep on having some kind of "report_version" key still help if in the future we'd want to change this format again? Having a version string in there could make it easier to support multiple versions?

I talked a bit with Chris about this. And I think we want to persue a somwhat weaker version scheme were tools add something like "generated": "LNT version 0.4.1" or "generated": "llvm test-suite runner 0.1", similar in spirit to the http user agent header.

I'm not sure I fully get this. I know of at least 2 groups having custom scripts to translate data coming out of their custom benchmarking system into something that can be imported into LNT. I know of yet another group planning to do something similar. I think it won't be possible for LNT to understand all possible producers of json files by name. So, with my limited insight here, I think that just a numerical version number or something similar indicating which version of the specification this json follows would be better than a string naming the producer?
In other words, I think this json format is rapidly becoming one of the key public interfaces of LNT - maybe the single most important one. If LNT keeps on growing in popularity in teams outside of LLVM, I think we'll end up having to specify this interface, and version control of it, a bit more strictly.

Ok, I'll add a field 'format_version' to the toplevel.

Thanks, sounds good.

  "tag": "nts",
  "run_order": "123456"
}

}
Now:

"machine": {

"name": "mymachine",
"hardware": "HAL 9000"

},
"run": {

"start_time": "2017-05-16 21:31:42",
"end_time": "2017-05-16 21:31:42",
"llvm_project_revision": "123456"

Since I see LNT being used also for tracking performance for non-LLVM projects, I think it'd be best to drop "llvm_" from "llvm_project_revision".

• The name for the field is determined by the schema in LNT, so not necessarily part of this patch.

OK, I agree it doesn't have to be part of this patch. But taking into account my remark above about the json file rapidly becoming one of the most important interfaces of LNT, I think that it may not be tenable in the long run to have the json fields map exactly to names of fields in the database schema. It is of course a nice to have; but the field names in the json file are on the publicly-visible interface of LNT, and the database field names are an implementation detail from a user point-of-view. I can foresee reasons why both might start deviating.
Since my gut feel is that it will be harder/more work to change the "json format specification" in the future, I thought that maybe it could be part of the change that is going to happen as part of this patch. Anyway, I don't have too strong a feeling about the exact name of this field.

Yes and no; As see above about my vision of actually using the LNT feature of different benchmark suites instead of pushing everything into NTS :)

Right. As you said earlier, probably the only teams using different benchmark suite schemas may well be in Apple, so me and others using LNT elsewhere probably don't have the best feel for this.
I'm not against keeping the DB scheme as is. As you rightfully indicated, the key is making it easier to add fields for other test-suites.

• "project_revision" seems to generic to me (I wouldn't know whether it's the compiler or the benchmark suites revision). I could probably live with "compiler_revision", but we should have a separate patch for that.

Agreed, it's not the best possible name. It should be the revision of the software for which you're tracking the performance in my mind. That software doesn't have to be a "compiler". Not sure what a better name would be though.

I'd be fine changing it to "compiler_revision" right away, if we need more discussion about how to call the field, then I'd suggest doing that in another patch to not delay this patch.

Let's change it to "compiler_revision" then, as all the other projects I know of so far track performance of a piece of software that could be claimed to be a compiler of sorts.

The change LGTM, but please keep it on topic (see below).

• Actually I have one more request: Could you move libs/benchmark-1.1.0 to MicroBenchmarks/benchmark-1.1.0 (or MicroBenchmarks/libs/benchmark-1.10 if you prefer that). The library should only be used by microbenchmarks and that way people that do things like just running CTMark don't have to compile the library. It also saves you from modifying the toplevel cmake file and adding that exception for libs to the TEST_SUITE_SUBDIR list construction.

In D32272#794759, @dberris wrote:

In D32272#789252, @hfinkel wrote:

In D32272#789237, @MatzeB wrote:

• Todays lit works best if you have 1 file for each test; Also test discover is done before any test is run.
• If there is a flag to only run a specific benchmark in a googletest executable then we could work around this limitation on the cmake side:
  
  Just generate multiple .test files with slightly different RUN: lines (assume that option is called --only-test): ` retref-bench-unpatched.test contains RUN: retref-bench --only-test unpatched retref-bench-patched.test contains RUN: retref-bench --only-test patches ... ` It would require us to repeat the sub-benchmark names in the cmake file (or write cmake logic to scan the sourcecode?)

I suppose we could do this, but realistically, we need to be able to collect multiple named timings per test. We need some way of telling lit, "don't use the timing from this executable, it provides timing numbers some other way", and then some mechanism from parsing the output to collect those numbers. It seems fairly straightforward to do this as an LNT plugin because the benchmark library has its own well-defined output format, which is why I mentioned that, but could we build something into lit as well.

It might sound weird, but I agree with both of you. :)

On the one hand, we shouldn't need to introduce too much change all at once -- right now I'm happy to just get the benchmarks there, and have them be runnable by people who want to test this on different platforms. While they might not look like the rest of the benchmarks/tests in the test-suite already, I'd be very happy to iterate on it doing both of @MatzeB and @hfinkel's suggestions. More concretely:

• In the interim, create multiple .test files each running one of the named benchmarks in the single .cc file.
• In the future teach LNT to understand the Google Benchmark output.
• Once LNT knows how to interpret the data and we can set the running of the tests such that we can reduce variance (there's a mode for Google Benchmark that only shows mean times and standard deviation by providing time bounds and iteration bounds) then we can sanitise the multiple files per benchmark.
  
  Now for this particular change, is the current state and the plan to iterate seem reasonable to either of you (@MatzeB and @hfinkel)?

Fix typo in docu.

• Rebase to ToT
• Add "format_version": "2" to the toplevel dictionary.

Before I dive in the meta discussion, are there any issues with the patch itself?

Sure, LGTM.

In D34584#791693, @kristof.beyls wrote:

Thank you so much for this, Matthias!

Some questions/remarks inline:

This redesigns to get a new simpler import format for LNT. With plans to
use the same format for exporting.

Go for lowercase keys without spaces.

Makes sense to me.

Drop Run/Info/tag: json itself is a schemaless format so it feels more natural without one. The LNT testsuite type is now determined by the submit URL used, or the '-s' commandline flag on the commandline. Both default to 'nts' if omitted.

I don't fully understand how LNT reacts to this, but on the surface makes sense to me. If someone ever had time to do a major redesign of the database schema, there is a chance we might also be able to drop the need for differences in the database schema per test suite, like "nts"? Anyway, that is definitely far out of scope for this patch.

The patch works fine. People using non-nts testsuites (which I have a strong suspicion only exist within apple), may need to supply a -s MySchema flag for the commandline tools or use db_default/v4/MySchema/submitRun instead of db_default/submitRun to express they don't use the 'nts' default.

Thanks, LGTM.

In D34362#790215, @cs14mtech11017 wrote:

In D34362#789280, @cmatthews wrote:

It makes more sense to me to be using the --make-param flag to pass a test specific configuration options. If you want to add all these size classes, all the tests should support them, or have them mapped back to nearest size the tests can handle.

I feel having a common flag for test data sizes is better than having to pass them as test specific flag. As you suggested, we can map them back to nearest data sizes for the other tests. I was working with Polybench, so did not look into other tests. Will proceed with it if everyone agrees on adding a flag like "--testdatasizes=mini|small|medium|large|extra_large" and aliasing --small and --large for backward compatibility.

LGTM

In D34581#790630, @kristof.beyls wrote:

In D34581#790607, @cmatthews wrote:

It would be wonderful if we could programmatically check these during testing. Thanks Matthias!

I had to make the tags XML-compliant in the daily report page to make testing its content in tests/server/ui/V4Pages.py possible using the Python built-in xml parser (see functions check_nr_machines_reported and get_xml_tree).
So, in other words, I think checking this programmatically would probably be easy and boil down to parsing every HTML page using the Python built-in xml parser, see function get_xml_tree pointed to in the sentence above.

I think this landed in r294268

In D34445#791477, @kevans91 wrote:

In D34445#791469, @MatzeB wrote:

I'd love that and requiring posix in the test-suite should be fine. Why does your patch check FreeBSD defines if this is a posix API?

Mostly to be as uninvasive as possible, but also since I don't personally have the capacity to double-check on libc's other than FreeBSD's and glibc to a lesser extent. I can re-work it, but I do wonder if Solaris' implementation now matches POSIX.

The code fix LGTM, but please wait for @qcolombet/@arsenm before committing.

I'm fine with the review as is, to get things working on FreeBSD.

While I think you are fine here, you should probably double check that TargetRegisterInfo::getMinimalPhysRegClass() does not start returning your new register class for the "normal" go regs or if it does at least keep an eye on the performance bots to make sure it doesn't accidentally change.

From what I remember the notion of a nullptr in Map for a block we had already Seen was introduced with the changes supporting undefs when recalculating liveness. I assumed that had the same role as the special VNI you introduce here. Can you elaborate the difference (and/or eliminate the cases looking for nullptr)?

Thanks, LGTM!

LGTM

Add missing file

Fix comment in server_wrapper.sh

In D32272#789224, @hfinkel wrote:

In D32272#789201, @MatzeB wrote:

In D32272#789021, @hfinkel wrote:

In D32272#789012, @kristof.beyls wrote:

In D32272#789006, @hfinkel wrote:

In D32272#788932, @dberris wrote:

LGTM for the most part, thanks for making the changes, it's much simpler now.

Thoughts @MatzeB? I think we can work on running/reporting the microbenchmarks data later, in changes to LNT.

Can you please summarize the state of this and the eventual goal? It looks like:

1. Eventually, we need a new LNT plugin (i.e. something like what we have in LNTBased/*) that understands how to interpret the output of the Google benchmark library.

I hope this could all be done within the test-suite, probably extending test-suite/litsupport. I hope/think that no changes to LNT will be needed, but I haven't investigated myself.

Just to be clear, LNTBased/* is in the test suite; LNT supports plugins and those can be in the test suite. That having been said, if we can do this just within litsupport, that potentially seems better. The key feature seems to be being able to report multiple benchmark timings per actual executable.

• The lit data format currently does not allow multiple values for a metric.
• I'm not sure I see why that would be needed here.

I don't mean multiple times for each benchmark, I mean that there are multiple benchmarks for which we should individually collect timings. In the benchmark added here, for example, we'll get individual timings out for:

BENCHMARK(BM_ReturnInstrumentedu);
BENCHMARK(BM_ReturnInstrumentedPatchedThenUnpatched);
BENCHMARK(BM_ReturnInstrumentedPatched);
...

and we should separately record them all.

In D32272#788932, @dberris wrote:

LGTM for the most part, thanks for making the changes, it's much simpler now.

Thoughts @MatzeB? I think we can work on running/reporting the microbenchmarks data later, in changes to LNT.

In D32272#789021, @hfinkel wrote:

In D32272#789012, @kristof.beyls wrote:

In D32272#789006, @hfinkel wrote:

In D32272#788932, @dberris wrote:

LGTM for the most part, thanks for making the changes, it's much simpler now.

Thoughts @MatzeB? I think we can work on running/reporting the microbenchmarks data later, in changes to LNT.

Can you please summarize the state of this and the eventual goal? It looks like:

1. Eventually, we need a new LNT plugin (i.e. something like what we have in LNTBased/*) that understands how to interpret the output of the Google benchmark library.

I hope this could all be done within the test-suite, probably extending test-suite/litsupport. I hope/think that no changes to LNT will be needed, but I haven't investigated myself.

Just to be clear, LNTBased/* is in the test suite; LNT supports plugins and those can be in the test suite. That having been said, if we can do this just within litsupport, that potentially seems better. The key feature seems to be being able to report multiple benchmark timings per actual executable.

• We should really add a litsupport/ plugin rather than starting an LNTBased suite. I see no advantage of using LNTBased, but at the same time writing a new runner from scratch means you have to reimplement (or not implement) codesize, compiletime measurements, pgo data collection feeding back into the build etc.

• Indeed, active test-suite development is done in the cmake build (test_suite.py in lnt). You may still change nt.py but landing features exclusively there would be bad.
• It seems the llvm test-suite today only checks for SMALL_PROBLEM_SIZE, occasionally LARGE_PROBLEM_SIZE and for SPEC you can choose the size manually. Do you plan to change the test-suite for the new sizes? We should probably do further discussions there.
• Do you plan to set any rules/expectations on those new sizes? (Something like "mini" finished in half the time than "small")
• Out of interest: Why do you need finer control over the problem sizes?

In D34464#787900, @chapuni wrote:

I have reproduced the issue, and confirmed this works.

Yet another option; Could we make ProgressBar accept unicode?
The difference is; sys.stdout.write() doesn't accept unicode (by default) but print() accepts.

Thanks for working on this! A bunch of nitpicks are below but overal the fix looks fine.

D34464 fixes this in a less hacky way.

For the record if someone wants to reproduce:

LGTM