MatzeB (Matthias Braun)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 13 2013, 3:10 PM (205 w, 2 d)

Recent Activity

Yesterday

MatzeB accepted D35446: Add an ID field to StackObjects.

LGTM

Thu, Jul 20, 12:15 PM
MatzeB added a comment to D35688: More extendable LaneBitmask.
  • 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.
Thu, Jul 20, 10:49 AM

Wed, Jul 19

MatzeB added a comment to D35414: [Support] [IR] [ADT] - Check nullptr after allocation with malloc/realloc or calloc.

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.

Wed, Jul 19, 6:32 PM
MatzeB requested changes to D34889: [ScheduleDAG] Fix bug in check for use of dead defs.

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

Wed, Jul 19, 5:42 PM
MatzeB accepted D34897: Replace -print-whole-regmask with a threshold..

LGTM

Wed, Jul 19, 4:57 PM
MatzeB added a reviewer for D35446: Add an ID field to StackObjects: qcolombet.

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

Wed, Jul 19, 4:55 PM
MatzeB added a comment to D35073: [RegisterCoalescer] Fix for subrange join unreachable.

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.

Wed, Jul 19, 4:48 PM
MatzeB added a comment to D35229: [CodeGen] Add support for instruction clusters.

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
Wed, Jul 19, 4:32 PM
MatzeB accepted D35642: [PEI] Simplify handling of targets with no phys regs. NFC.

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.

Wed, Jul 19, 4:26 PM
MatzeB accepted D35644: [PEI] Separate saving and restoring CSRs into different functions. NFC.

LGTM, style suggestions below.

Wed, Jul 19, 4:25 PM
MatzeB abandoned D12815: RegisterPressure: Move MaxSetPressure from RegisterPressure to PressureTracker.
Wed, Jul 19, 4:20 PM
MatzeB abandoned D12816: MachineScheduler: Integrate RegisterPressure classes into PressureTracker.
Wed, Jul 19, 4:17 PM
MatzeB abandoned D24855: MachineScheduler: Enable macro fusion in post-RA scheduler.

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.

Wed, Jul 19, 4:16 PM
MatzeB closed D31145: [Outliner] Fix compile-time overhead for candidate choice.

This seems to have landed a while ago in r298648

Wed, Jul 19, 4:13 PM

Tue, Jul 18

MatzeB created D35600: Remove --commit/--commit=0 flags/settings..
Tue, Jul 18, 6:35 PM
MatzeB created D35598: Rework machine creation strategy.
Tue, Jul 18, 6:30 PM
MatzeB added a comment to D35422: Add MemoryMappedSection struct for two-level memory map iteration.

Reverted as rL308394 and rL308395

Tue, Jul 18, 4:54 PM
MatzeB created D35588: Remove --commit/--commit=1 flags/settings..
Tue, Jul 18, 4:37 PM
MatzeB added a comment to D35422: Add MemoryMappedSection struct for two-level memory map iteration.
...
PASS: AddressSanitizer-x86_64-darwin :: TestCases/Darwin/dump_registers.cc (911 of 2593)
Build timed out (after 45 minutes). Marking the build as aborted.
Set build name.
New build name is 'r308348 (#17551)'
Variable with name 'BUILD_DISPLAY_NAME' already exists, current value: 'r308348 (#17551)', new value: 'r308348 (#17551)'
Build was aborted
Recording test results
ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?
Tue, Jul 18, 4:00 PM
MatzeB added a comment to D35422: Add MemoryMappedSection struct for two-level memory map iteration.

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?

Tue, Jul 18, 3:56 PM
MatzeB updated the diff for D35501: Add lnt commandline REST client.
  • Add unittests
  • Add documentation
Tue, Jul 18, 12:14 PM
MatzeB created D35575: LNT Documentation Cleanup.
Tue, Jul 18, 11:38 AM

Mon, Jul 17

MatzeB accepted D35366: [AArch64][Falkor] Avoid HW prefetcher tag collisions (step 2).

LGTM

Mon, Jul 17, 8:38 PM
MatzeB created D35501: Add lnt commandline REST client.
Mon, Jul 17, 12:58 PM
MatzeB added a comment to D35313: [libFuzzer] Add a dependency on symbolizer from libFuzzer tests.

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?

Mon, Jul 17, 11:04 AM

Sun, Jul 16

MatzeB added inline comments to D34963: [AArch64][Falkor] Avoid HW prefetcher tag collisions (step 1).
Sun, Jul 16, 10:35 AM
MatzeB added inline comments to D35366: [AArch64][Falkor] Avoid HW prefetcher tag collisions (step 2).
Sun, Jul 16, 10:26 AM

Fri, Jul 14

MatzeB accepted D35414: [Support] [IR] [ADT] - Check nullptr after allocation with malloc/realloc or calloc.

LGTM

Fri, Jul 14, 9:17 AM

Wed, Jul 12

MatzeB added a comment to D35287: llc: Require an explicit -mtriple=default to target default triple.

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

Wed, Jul 12, 3:39 PM

Tue, Jul 11

MatzeB created D35287: llc: Require an explicit -mtriple=default to target default triple.
Tue, Jul 11, 6:41 PM
MatzeB accepted D35272: [X86/FastIsel] Fall-back to SelectionDAG when lowering soft-floats.

LGTM

Tue, Jul 11, 1:49 PM

Mon, Jul 10

MatzeB accepted D35231: [CodeGen] Rename DEBUG_TYPE to match passnames.

Yep, that was an oversight. Thanks for fixing!

Mon, Jul 10, 7:40 PM
MatzeB added a comment to D35228: [TableGen] Add support for instruction clusters.

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?

Mon, Jul 10, 7:39 PM
MatzeB accepted D35144: [CodeGen] Add dependency printer.

LGTM

Mon, Jul 10, 7:33 PM
MatzeB added a comment to D33345: [DAG] Improve Aliasing of operations to static alloca.

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

Mon, Jul 10, 1:41 PM
MatzeB accepted D35105: [SjLj] Replace recursive block marking algorithm with iterative algorithm.

Looks good to me. Possible improvement below.

Mon, Jul 10, 1:30 PM
MatzeB added a comment to D34753: [Support] - Add bad alloc error handler for handling allocation malfunctions.

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).

Mon, Jul 10, 11:36 AM
MatzeB added a comment to D34753: [Support] - Add bad alloc error handler for handling allocation malfunctions.

So how about

#include <new>
...
x = malloc(...);
if (x == nullptr)
  throw std::bad_alloc();
Mon, Jul 10, 11:34 AM

Thu, Jul 6

MatzeB added a comment to D34962: [TargetLowering] Add hook for adding target MMO flags when doing ISel..

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

Thu, Jul 6, 8:34 PM
MatzeB requested changes to D35055: [InlineSpiller] Only examine defs in BUNDLE instruction.

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

Thu, Jul 6, 2:02 PM

Wed, Jul 5

MatzeB added a comment to D34753: [Support] - Add bad alloc error handler for handling allocation malfunctions.

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.

Wed, Jul 5, 10:40 AM

Fri, Jun 30

MatzeB added a comment to D34910: Make LLVM_TARGETS_TO_BUILD=all build all targets.

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

Fri, Jun 30, 5:37 PM
MatzeB added a comment to D34910: Make LLVM_TARGETS_TO_BUILD=all build all targets.

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.

Fri, Jun 30, 4:44 PM
MatzeB added a comment to D34910: Make LLVM_TARGETS_TO_BUILD=all build all targets.

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

Fri, Jun 30, 2:33 PM
MatzeB added a comment to D34897: Replace -print-whole-regmask with a threshold..

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.

Fri, Jun 30, 12:09 PM
MatzeB added a comment to D34897: Replace -print-whole-regmask with a threshold..

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

Fri, Jun 30, 12:05 PM
MatzeB added a reviewer for D34897: Replace -print-whole-regmask with a threshold.: qcolombet.
Fri, Jun 30, 11:56 AM
MatzeB added a comment to D34897: Replace -print-whole-regmask with a threshold..

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.
Fri, Jun 30, 11:56 AM
MatzeB accepted D32272: [XRay] Add Google Benchmark library + initial XRay benchmarks.

See comments below but nothing blocking. LGTM.

Fri, Jun 30, 11:26 AM

Thu, Jun 29

MatzeB added inline comments to D34586: Remove info_keys; NFC.
Thu, Jun 29, 4:05 PM
MatzeB added a comment to D34830: [lit, test-suite] Fix a reference to FileBasedTest that I missed earlier..
In D34830#795804, @dlj 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.

Thu, Jun 29, 1:49 PM
MatzeB added a comment to D34584: Introduce new JSON import format.

Thanks for the review!

Thu, Jun 29, 1:47 PM
MatzeB updated subscribers of D34830: [lit, test-suite] Fix a reference to FileBasedTest that I missed earlier..

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?

Thu, Jun 29, 1:30 PM
MatzeB added a comment to D34584: Introduce new JSON import format.

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.

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.

Thu, Jun 29, 11:51 AM
MatzeB added inline comments to D34793: [lit] Fix some convoluted logic around Unicode encoding, and de-duplicate across modules that used it..
Thu, Jun 29, 10:23 AM

Wed, Jun 28

MatzeB added a comment to D34793: [lit] Fix some convoluted logic around Unicode encoding, and de-duplicate across modules that used it..

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

Wed, Jun 28, 10:02 PM
MatzeB added a comment to D32272: [XRay] Add Google Benchmark library + initial XRay benchmarks.
  • 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.
Wed, Jun 28, 9:37 PM
MatzeB added a comment to D32272: [XRay] Add Google Benchmark library + initial XRay benchmarks.
  • 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)?
Wed, Jun 28, 9:35 PM
MatzeB added inline comments to D32272: [XRay] Add Google Benchmark library + initial XRay benchmarks.
Wed, Jun 28, 8:29 PM
MatzeB updated the diff for D34584: Introduce new JSON import format.

Fix typo in docu.

Wed, Jun 28, 4:50 PM
MatzeB updated the diff for D34584: Introduce new JSON import format.
  • Rebase to ToT
  • Add "format_version": "2" to the toplevel dictionary.
Wed, Jun 28, 4:49 PM
MatzeB added a comment to D34584: Introduce new JSON import format.

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

Wed, Jun 28, 4:04 PM

Tue, Jun 27

MatzeB added inline comments to D34713: [TableGen] Improve Debug Output for --debug-only=subtarget-emitter NFCI.
Tue, Jun 27, 5:01 PM
MatzeB accepted D34713: [TableGen] Improve Debug Output for --debug-only=subtarget-emitter NFCI.

Sure, LGTM.

Tue, Jun 27, 3:45 PM
MatzeB added inline comments to D34640: Create a PHI value when merging with a known undef live-in.
Tue, Jun 27, 2:42 PM
MatzeB added a comment to D34584: Introduce new JSON import format.

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.

Tue, Jun 27, 2:36 PM
MatzeB accepted D34640: Create a PHI value when merging with a known undef live-in.

Thanks, LGTM.

Tue, Jun 27, 1:50 PM
MatzeB added a comment to D34362: [LNT] Support for different DataSet usage in Polybench for "lnt runtest nt".

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.

Tue, Jun 27, 1:07 PM
MatzeB accepted D34700: RenameIndependentSubregs: Fix infinite loop.

LGTM

Tue, Jun 27, 10:46 AM

Mon, Jun 26

MatzeB added a comment to D34581: Fix missing/mismatched html tags.

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.

Mon, Jun 26, 9:43 PM
MatzeB closed D29611: RegisterCoalescer: Fix joinReservedPhysReg().

I think this landed in r294268

Mon, Jun 26, 7:51 PM
MatzeB added a comment to D34445: Link against libcompat on FreeBSD if re_comp is used.

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.

Mon, Jun 26, 6:31 PM
MatzeB accepted D34391: [RegisterCoalescer] Fix for SubRange join unreachable.

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

Mon, Jun 26, 6:25 PM
MatzeB added inline comments to D34445: Link against libcompat on FreeBSD if re_comp is used.
Mon, Jun 26, 6:17 PM
MatzeB accepted D34445: Link against libcompat on FreeBSD if re_comp is used.

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

Mon, Jun 26, 6:15 PM
MatzeB accepted D34610: [ARM] Add tGPRwithpc register class and use it for TBB/THH.

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.

Mon, Jun 26, 4:40 PM
MatzeB added a comment to D34640: Create a PHI value when merging with a known undef live-in.

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)?

Mon, Jun 26, 4:33 PM
MatzeB accepted D34643: RenameIndependentSubregs: Fix iterator problem.

Thanks, LGTM!

Mon, Jun 26, 1:53 PM
MatzeB accepted D34109: [AArch64] Update successor probabilities after ccmp-conversion.

LGTM

Mon, Jun 26, 10:44 AM

Fri, Jun 23

MatzeB created D34587: developer_guide: Tests need lnt in $PATH.
Fri, Jun 23, 5:20 PM
MatzeB added a dependency for D34586: Remove info_keys; NFC: D34584: Introduce new JSON import format.
Fri, Jun 23, 5:18 PM
MatzeB added a dependent revision for D34584: Introduce new JSON import format: D34586: Remove info_keys; NFC.
Fri, Jun 23, 5:18 PM
MatzeB created D34586: Remove info_keys; NFC.
Fri, Jun 23, 5:18 PM
MatzeB updated the diff for D34584: Introduce new JSON import format.

Add missing file

Fri, Jun 23, 5:12 PM
MatzeB created D34584: Introduce new JSON import format.
Fri, Jun 23, 5:10 PM
MatzeB created D34581: Fix missing/mismatched html tags.
Fri, Jun 23, 4:45 PM
MatzeB updated the diff for D34572: Let lnt submit print the URL of the resulting run.

Fix comment in server_wrapper.sh

Fri, Jun 23, 2:29 PM
MatzeB created D34572: Let lnt submit print the URL of the resulting run.
Fri, Jun 23, 2:27 PM
MatzeB added a comment to D32272: [XRay] Add Google Benchmark library + initial XRay benchmarks.

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.

Fri, Jun 23, 12:08 PM
MatzeB added a comment to D32272: [XRay] Add Google Benchmark library + initial XRay benchmarks.

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.

Fri, Jun 23, 11:52 AM
MatzeB added a comment to D32272: [XRay] Add Google Benchmark library + initial XRay benchmarks.

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.

Fri, Jun 23, 11:45 AM
MatzeB added a comment to D32272: [XRay] Add Google Benchmark library + initial XRay benchmarks.
  • 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.
Fri, Jun 23, 11:41 AM
MatzeB added a comment to D34362: [LNT] Support for different DataSet usage in Polybench for "lnt runtest nt".
  • 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?
Fri, Jun 23, 11:19 AM

Thu, Jun 22

MatzeB added a comment to D34464: lit: Make sure testnames are unicode strings.

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.

Thu, Jun 22, 11:19 AM

Wed, Jun 21

MatzeB added a comment to D34391: [RegisterCoalescer] Fix for SubRange join unreachable.

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

Wed, Jun 21, 5:17 PM
MatzeB added inline comments to D34464: lit: Make sure testnames are unicode strings.
Wed, Jun 21, 3:04 PM
MatzeB added a comment to D33468: Enable unicode output on terminals..

D34464 fixes this in a less hacky way.

Wed, Jun 21, 10:56 AM
MatzeB added a comment to D34464: lit: Make sure testnames are unicode strings.

For the record if someone wants to reproduce:

Wed, Jun 21, 10:56 AM
MatzeB created D34464: lit: Make sure testnames are unicode strings.
Wed, Jun 21, 10:53 AM

Jun 20 2017

MatzeB accepted D34320: Use range-loop in machine-scheduler. NFCI..

LGTM

Jun 20 2017, 8:41 PM