MatzeB (Matthias Braun)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 13 2013, 3:10 PM (214 w, 4 d)

Recent Activity

Fri, Sep 22

MatzeB added a comment to D38164: [MachineScheduler] Favor instructions that do not increase pressure..
  • I vaguely remember trying something like this and having some crypto benchmarks produce bad schedules; I'll see if I can remember/find it
  • tryPressure seems like the wrong place to me, as it is used in 3 different contexts: (compared with target limits, compared with increase region limits, and the current max). From your description it sounds like we only want this behavior once.
  • Do you have a specific example where this helps?
Fri, Sep 22, 11:57 AM

Thu, Sep 21

MatzeB added a comment to D23601: [TII] add new target hook isAdd.

It would be great if someone could add more comments of what isAdd entails. All documentation in the current llvm code is a variation of Is this instruction an add instruction?/\brief Return true if the instruction is an add instruction.. Are there any requirements on which operands of the instruction are actually added together or hold the result? Can they be immediates and register operands, load from memory?

Thu, Sep 21, 1:57 PM

Wed, Sep 20

MatzeB added inline comments to D36721: [MachineOutliner] AArch64: Avoid saving + restoring LR if possible.
Wed, Sep 20, 7:01 PM
MatzeB added a comment to D37562: [X86] Lower _mm[256|512]_[mask[z]]_avg_epu[8|16] intrinsics to native llvm IR.

Was the code not using emmintrin.h and instead copied code from it that used the builtins?

Wed, Sep 20, 5:42 PM
MatzeB added a comment to D37562: [X86] Lower _mm[256|512]_[mask[z]]_avg_epu[8|16] intrinsics to native llvm IR.

Actually, looking at the change it seems like it is and we better try to update emmintrin.h...

Wed, Sep 20, 5:38 PM
MatzeB added a comment to D37562: [X86] Lower _mm[256|512]_[mask[z]]_avg_epu[8|16] intrinsics to native llvm IR.

With this commit some (old) code stops to compile as it seems to remove some builtins, was that intentional?

Wed, Sep 20, 5:36 PM
MatzeB updated the diff for D38106: TargetLibraryInfo: Stop guessing the wchar_t size.

Add a unittest to ensure we are indeed not guessing the wchar_t size.

Wed, Sep 20, 3:59 PM
MatzeB added a comment to D38106: TargetLibraryInfo: Stop guessing the wchar_t size.

After thinking about it some more, I decided that we better leave the logic to decide on the wchar_t size to a C frontend.

Wed, Sep 20, 3:58 PM
MatzeB created D38106: TargetLibraryInfo: Stop guessing the wchar_t size.
Wed, Sep 20, 3:55 PM
MatzeB accepted D38091: [TableGen] Tidy up CodeGenRegisters.

LGTM

Wed, Sep 20, 1:58 PM

Tue, Sep 19

MatzeB added a comment to D37421: [XRay] [test-suite] Add LNT support to retref-bench benchmarks..

I'll write some paragraphs on how to get started in the README file and make sure the interfaces have better documentation comments.

Tue, Sep 19, 8:03 PM
MatzeB added a comment to D37421: [XRay] [test-suite] Add LNT support to retref-bench benchmarks..

I found two problems with this at the moment:

  • The list of litsupport plugins is currently global. Thus enabling this microbenchmarking plugin will make the non-microbenchmarking tests fail. It should be easy enough to change this to a local setting so that lit.local.cfg files can override the plugin list (just have to convince myself that this is a good feature to have :).

What's the design space here? Would it be better to be able to name plugins in the .test file?

Tue, Sep 19, 8:01 PM
MatzeB added a comment to D37421: [XRay] [test-suite] Add LNT support to retref-bench benchmarks..

First: This looks ok and can be committed as is if you want.

However IMO using the LNTBased system here is not the best choice, I'd rather try writing a litsupport plugin. LNTBased is little used and IMO less flexible than the cmake+lit plugin approach.

I agree, having this in litsupport seems better (and also solves the duplication/symlink problem). I imagine that the plugin could append a function to metric_collectors that reads the CSV and reports the associated timing data. @MatzeB, is there documentation on the litsupport plugin interface anywhere? (if all we have is what's in the README.md, we should write some more).

Tue, Sep 19, 5:05 PM
MatzeB added a comment to D35396: [lit] Remove %T.

While at it, could the documentation also be clarified whether %t refers to a prefix (or note that it is created in a subdirectory such that one can know for sure that it is cleaned up)? A lot of tests use something like %clang -o %t.o.

%t is just a name/string that is based on the tests name and is therefore guaranteed to be unique for each test. There is no automatic cleanup, in fact I believe you don't want automatic cleanup as you want to inspect the intermediate results in case of errors.
However I believe the %t path is always in a subdirectory called Output so if you wanted to cleanup intermediate results between test-suite runs you could do something like find build/test -d -name "Output" -exec rm -rf {} ';'

Tue, Sep 19, 4:48 PM

Mon, Sep 18

MatzeB added a comment to D37421: [XRay] [test-suite] Add LNT support to retref-bench benchmarks..

First: This looks ok and can be committed as is if you want.

Mon, Sep 18, 3:47 PM

Fri, Sep 15

MatzeB added inline comments to D37891: Driver: hoist the `wchar_t` handling to the driver.
Fri, Sep 15, 5:02 PM · Restricted Project

Thu, Sep 14

MatzeB added a comment to D37611: [IfConversion] More simple, correct dead/kill liveness handling.

I found and fixed the reason our randomized tests didn't run cleanly. It was essentially that stepBackwards removed defined regs from the set even if the instruction was predicated.

It's supposed to do that. This is exactly the reason why we add implicit uses to predicated instructions if there is a live def reaching it.

Thu, Sep 14, 9:37 AM

Wed, Sep 13

MatzeB accepted D37698: Allow target to decide when to cluster loads/stores in misched.

LGTM

Wed, Sep 13, 2:43 PM
MatzeB added a comment to D37698: Allow target to decide when to cluster loads/stores in misched.
  • When seeing the name doMemOpsHaveSameBasePtr I would expect this to do exactly BaseReg1 == BaseReg2. Can you explain how you happen to have cases with different base pointers that still reference the same object?

That can only work if instruction has base register and offset operands. AMDGPU flat_load and flat_store instructions do not, their address operand is a single register. If you need to load two values from the same base pointer you do:

flat_load %ptr1
%ptr2 = %ptr1 + offset
flat_load %ptr2

  • Given that this callback is very specific to the selectiondag clustering mutator, maybe we better add a callback to createLoadClusterDAGMutation/createStoreClusterDAGMutation rather than in TargetInstructionInfo. That way you could also choose a better name like shouldCluster.

I cannot directly inherit LoadClusterMutation or StoreClusterMutation, it is defined in the MachineScheduler.cpp, and then if I move it to common interface I do not want to duplicate the code in both. I can pass a lambda/functor into create(Load|Store)ClusterDAGMutation as a last argument. Is that what you have in mind?

While you could just as well move the class declaration into a header an extra function argument is probably easiest.

Wed, Sep 13, 1:53 PM
MatzeB accepted D37756: [lit] Force site configs to be run before source-tree configs.

This turned out to be a simple directed change. Looks good to me.
(But maybe wait for further feedback).

Wed, Sep 13, 11:06 AM
MatzeB added a comment to D37698: Allow target to decide when to cluster loads/stores in misched.
  • When seeing the name doMemOpsHaveSameBasePtr I would expect this to do exactly BaseReg1 == BaseReg2. Can you explain how you happen to have cases with different base pointers that still reference the same object?
  • Given that this callback is very specific to the selectiondag clustering mutator, maybe we better add a callback to createLoadClusterDAGMutation/createStoreClusterDAGMutation rather than in TargetInstructionInfo. That way you could also choose a better name like shouldCluster.
  • We should indeed not change the behavior for aarch64, as the mutation mainly exists there to enable formation of load/store double instructions which require the same base pointer.
Wed, Sep 13, 10:45 AM
MatzeB accepted D37611: [IfConversion] More simple, correct dead/kill liveness handling.

LGTM with nitpicks.

Wed, Sep 13, 10:34 AM
MatzeB added a comment to D37611: [IfConversion] More simple, correct dead/kill liveness handling.

To add to my previous comment---there is a plan to eliminate kill flags in the future (as I was told), so passes the rely on those will eventually need to be modified. If it's the presence of kill flags on predicated instructions (and tied uses) that is causing problems, maybe it would be a good thing to address that in the passes that are exploiting that to generate wrong code. It would imply more work, so I guess the answer depends on how much more work it would be.

Wed, Sep 13, 10:30 AM
MatzeB added a comment to D37611: [IfConversion] More simple, correct dead/kill liveness handling.

I just started a test run on a Hexagon suite, but I wonder if the anti-dependence breaker could be a problem. This is regarding the question of whether the implicit uses on predicated instructions should be killed or not. BTW, the same argument applies to tied uses. Even though the live ranges seem to end/start at that instruction, the register cannot be renamed in only one of them. This differentiates those instructions from a case of unrelated use/def coincidentally using the same register (as was in the example: r0 = add r0, r1).

Wed, Sep 13, 10:27 AM

Tue, Sep 12

MatzeB accepted D37748: [MiSched|TableGen] : Tidy up and modernise. NFC..

LGTM with nitpicks.

Tue, Sep 12, 9:58 AM

Mon, Sep 11

MatzeB added inline comments to D37611: [IfConversion] More simple, correct dead/kill liveness handling.
Mon, Sep 11, 10:09 AM

Fri, Sep 8

MatzeB added a comment to D37611: [IfConversion] More simple, correct dead/kill liveness handling.

This is a very good idea!

Fri, Sep 8, 10:57 AM
MatzeB added a comment to D33650: MachineVerifier: Verify liveins list.

Make sure that at the end of basic blocks the liveins of the successor blocks are live (or the CSRs in case of return blocks)

So this only checks that there are not any false live-in entries in the live-in lists of successors, meaning that there could still be missing regs in the live-in lists of successors?

Missing regs will already be catched (use of an undefined register). The important check here is that regs marked live-in are actually alive at the end of the predecessor blocks.

Fri, Sep 8, 9:24 AM
MatzeB accepted D37600: Preserve existing registers when adding pristines to LivePhysRegs.

LGTM, thanks

Fri, Sep 8, 9:16 AM

Thu, Sep 7

MatzeB added a comment to D33960: ErrorHandling: report_fatal_error: call abort() instead of exit().

Did a quick ad-hoc and incomplete survey of report_fatal_error usage in LLVM.

Thu, Sep 7, 5:31 PM
MatzeB added a comment to D33960: ErrorHandling: report_fatal_error: call abort() instead of exit().

I think the question is, is a report_fatal_error exit indicative of a bug, or is it possibly working as intended? For a long time we've treated it as a valid (but lazy and crappy) way to handle errors in library code. Are we changing that? I could be OK with that as long as its announced. If we want to do this, we should announce it as a new direction for the project on llvm-dev.

I feel like almost every time I have hit report_fatal_error, I have wanted it to break in a debugger and potentially provide a stack trace. Maybe I'm weird though.

Exactly the same for me and that was my motivation for this patch.

Thu, Sep 7, 5:13 PM
MatzeB added a comment to D37600: Preserve existing registers when adding pristines to LivePhysRegs.

When I wrote this I assumed that addLiveOuts()/addLiveIns() is only called on empty sets looks like I didn't add an assert() for that and ifconversion does indeed call it no non-empty sets now. Thanks for tracking this down!

Thu, Sep 7, 4:30 PM
MatzeB added a comment to D37361: Cleanup session creation code, use close() instead of rollback().

ping.
@cmatthews: I actually sneaked this change into our internal server and it is running fine for a while now.

Thu, Sep 7, 12:12 PM
MatzeB added a comment to D17257: New utility class ReachingPhysDefs for post-ra analysis..

Krzysztofs rdf patch is here https://reviews.llvm.org/D29295 and it would good if it got some reviewers and users.

Thu, Sep 7, 9:30 AM

Wed, Sep 6

MatzeB added a comment to D33960: ErrorHandling: report_fatal_error: call abort() instead of exit().

Hrm.

Seeing that closing stderr test - I wouldn't mind some more eyes on this change. There's a fair amount of report_fatal_error usage in LLVM tools that probably aren't abort-worthy? But I'm not sure. But maybe that's OK (I mean they're mostly internal tools anyway)

Wed, Sep 6, 4:53 PM
MatzeB updated subscribers of D33960: ErrorHandling: report_fatal_error: call abort() instead of exit().
Wed, Sep 6, 4:42 PM
MatzeB requested review of D33960: ErrorHandling: report_fatal_error: call abort() instead of exit().
Wed, Sep 6, 4:41 PM
MatzeB updated the diff for D33960: ErrorHandling: report_fatal_error: call abort() instead of exit().

Turns out this affects a bunch of testcases. Updating them here.

Wed, Sep 6, 4:41 PM
MatzeB added a comment to D37083: LNT Make machine selection/update more flexible.

Thanks for this. I don't know LNT well enough to review the code, but the top-level summary makes sense to me.

@leandron might be able to give more feedback on the code itself.

Thanks for the fix, and sorry for my long delay in getting back to you.

Wed, Sep 6, 2:49 PM

Tue, Sep 5

MatzeB updated the diff for D37034: Insert IMPLICIT_DEFS for undef uses in tail merging.

Rework branchfolding code to behave like this:

  • Make sure all the undef flags are merged
  • Compute new live-ins to NewDest (but do not set them yet)
  • Insert IMP_DEFS into NewDest predecessors where LiveOuts mismatch the new live-ins
  • Set new live-in of NewDest
  • For all other blocks with common tail:
    • Compute liveness at the position where we will cut the common tail and place the branch.
    • Insert IMP_DEFS as necessary where liveness mismatches live-ins of NewDest
    • Replace common tail with branch
Tue, Sep 5, 2:29 PM

Fri, Sep 1

MatzeB accepted D37401: [MIParser] Make sure that getHexUint doesn't produce APInts with a bitwidth of 0.

LGTM.

Fri, Sep 1, 2:44 PM
MatzeB added a comment to D37360: Keep sqlalchemy session separate from database objects.

I have felt before that LNT's connection and interaction with the database is too obscure in the code.
Therefore, I like this proposal, even if it means passing around session objects more.

I've even wondered before if we shouldn't just get rid of SqlAlchemy and write sql directly to allow us to speed up some of the heavy queries.
That would make the interaction with the database in LNT's code clearer again, removing an obfuscating layer of indirection.
However, I do see that with now supporting 3 database engines (sqlite, postgres, mysql), we may not want to have to reinvent the wheel on handling the database differences that SqlAlchemy already handles.
Anyway, the SqlAlchemy thought is clearly independent of this particular change.

I've been out of the loop on python & database interactions for too many years to be able to properly review this, but as said above, I do like the direction this is taking LNT's design/code base.

Fri, Sep 1, 1:38 PM
MatzeB added a comment to D37356: LiveIntervalAnalysis: Fix alias regunit reserved definition.

Thanks for the review!

Fri, Sep 1, 11:12 AM

Thu, Aug 31

MatzeB accepted D36621: [test-suite]: Adding SimpleMOC proxy-app..
  • MultiSource/Benchmarks/DOE-ProxyApps-C/SimpleMOC/LICENSE doesn't need to have the executable flag set
  • Otherwise integration LGTM.
Thu, Aug 31, 7:13 PM
MatzeB accepted D36626: [test-suite]: Adding RSBench proxy-app..

Integration LGTM.

Thu, Aug 31, 7:11 PM
MatzeB added a comment to D36628: [test-suite]: Adding Lulesh Proxy-app.

This fails verification for me on macOS:

Thu, Aug 31, 7:09 PM
MatzeB added a comment to D36629: [test-suite]: Adding lcals proxy-app.
  • No Makefile integration yet (though if this gets moved MicroBenchmarks, we cannot use Makefiles anyway, so this may be moot).
Thu, Aug 31, 7:03 PM
MatzeB accepted D36699: [test-suite] Adding miniGMG.

Integration LGTM.

Thu, Aug 31, 6:59 PM
MatzeB accepted D36718: [test-suite] Adding CLAMR.

Integration LGTM.

Thu, Aug 31, 6:56 PM
MatzeB created D37361: Cleanup session creation code, use close() instead of rollback().
Thu, Aug 31, 4:00 PM
MatzeB added a dependency for D37361: Cleanup session creation code, use close() instead of rollback(): D37360: Keep sqlalchemy session separate from database objects.
Thu, Aug 31, 4:00 PM
MatzeB added a dependent revision for D37360: Keep sqlalchemy session separate from database objects: D37361: Cleanup session creation code, use close() instead of rollback().
Thu, Aug 31, 4:00 PM
MatzeB added a reviewer for D37360: Keep sqlalchemy session separate from database objects: leandron.
Thu, Aug 31, 3:36 PM
MatzeB created D37360: Keep sqlalchemy session separate from database objects.
Thu, Aug 31, 3:35 PM
MatzeB added inline comments to D37356: LiveIntervalAnalysis: Fix alias regunit reserved definition.
Thu, Aug 31, 2:49 PM
MatzeB created D37356: LiveIntervalAnalysis: Fix alias regunit reserved definition.
Thu, Aug 31, 2:15 PM
MatzeB added a comment to D37034: Insert IMPLICIT_DEFS for undef uses in tail merging.

Are there any new developments?

Thu, Aug 31, 2:05 PM
MatzeB added a comment to D37339: [LNT] Invalid field being used on the samples graph API.

Ah yeah I had to abandon field.index in r312059; it can usually be replaced by TestSuiteDB.get_field_index(field). Doing it like proposed here is fine too (except for the if not field_index test being bad now as Chris already mentioned).

Thu, Aug 31, 12:41 PM

Wed, Aug 30

MatzeB accepted D37055: [ARM] Reverse PostRASched subtarget feature logic.

Thanks for looking into this again. This is indeed better. Did you choose the "negative" DisablePostRAScheduler feature over a "positive" FeaturePostRAScheduler just so it's less lines needed in the .td file? The positive variant seems slightly user friendlier and in line with aarch64 to me, even though it means adding the flag to every processor variant except for swift and cyclone.

Wed, Aug 30, 2:29 PM
MatzeB added a comment to D34581: Fix missing/mismatched html tags.

For the record: I just added an optional integration to pytidylib/tidy-html5 that checks lnt pages for html problems (r312061). It can be used with lnt -Dtidylib=1.

Very nice! Thanks for all the cleanups and improvements you've been making to LNT lately!

I guess there's a chance tidylib might be better than just aiming to parse XHTML as proper XML, as it may go beyond what an XML validator is capable of, by being written specifically for HTML?

I would have preferred simpler xml validation too, in fact that is what I tried first. In the end I failed with that approach because the WTForms library that we use only outputs HTML5 and has no way to produce XHTML, thus resulting in unclosed <input> tags without an good way to fix it. So various pages with forms on them will fail xml validation right now.

Wed, Aug 30, 1:23 PM

Tue, Aug 29

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

For the record: I just added an optional integration to pytidylib/tidy-html5 that checks lnt pages for html problems (r312061). It can be used with lnt -Dtidylib=1.

Tue, Aug 29, 4:04 PM

Mon, Aug 28

MatzeB accepted D37164: [ARM] Fix bug in ARMLoadStoreOptimizer when kill flags are missing..

LGTM

Mon, Aug 28, 11:59 AM
MatzeB accepted D37151: [AArch64] Adjust the cost model for Exynos M1 and M2.

LGTM

Mon, Aug 28, 11:55 AM
MatzeB added a comment to D37164: [ARM] Fix bug in ARMLoadStoreOptimizer when kill flags are missing..

If I understand correctly, the original code was checking for the kill flags just to short-circuit the if statement more quickly?

That's what it looks like to me: an attempt to reduce the calls to regsOverlap(). Or at least that would make sense for 'BaseKill'. Checking 'OffKill' I don't get unless 'Base' and 'Off' are somehow known to be related?

Indeed checking for the kill flags is odd and removing the checks looks good to me. However there may be something about the offset register: Won't we have the same problem when the first load overwrites the offset register? So maybe we need to check for an overlap with offsetreg as well (and have a bigger problem if base+offset get overridden?)

Mon, Aug 28, 10:10 AM
MatzeB added a comment to D37164: [ARM] Fix bug in ARMLoadStoreOptimizer when kill flags are missing..

If I understand correctly, the original code was checking for the kill flags just to short-circuit the if statement more quickly?

That's what it looks like to me: an attempt to reduce the calls to regsOverlap(). Or at least that would make sense for 'BaseKill'. Checking 'OffKill' I don't get unless 'Base' and 'Off' are somehow known to be related?

Mon, Aug 28, 9:38 AM

Aug 25 2017

MatzeB added a comment to D36911: TableGen: Add --gen-register-info-debug-dump.

LGTM too.
Same comment as Krzysztof, would be nice if the debug printing was more intuitively discoverable (e.g., -debug alongside with -gen-register-info).

The problem with -gen-register-info -debug is that I would still expect the C++ to be generated. So we would end up with the C++ register info on stdout and the debug information on stderr? That feels cumbersome.

Thinking about it some more -gen-register-info -o /dev/null -debug may be fine, I'll got with that.

Aug 25 2017, 10:27 AM
MatzeB added a comment to D36911: TableGen: Add --gen-register-info-debug-dump.

LGTM too.
Same comment as Krzysztof, would be nice if the debug printing was more intuitively discoverable (e.g., -debug alongside with -gen-register-info).

Aug 25 2017, 10:24 AM
MatzeB added a comment to D37151: [AArch64] Adjust the cost model for Exynos M1 and M2.
  • I'd leave out the if (Subtarget.getProcFamily() != AArch64Subtarget::ExynosM1) return false; check in isExynosShiftExtFast() if someone chose the Exynos scheduling he probably wants that behavior even if getProcFamily() doesn't match. (You can pull the check out to isAsCheapAsAMove()).
  • This must have happened in an earlier commit already, but in any case I'd really discourage the use of Subtarget.getProcFamily(), better create a subtarget feature to control the behavior.
Aug 25 2017, 10:14 AM
MatzeB accepted D37148: [LNT] Remove reference of lnt createdb from docs..

LGTM

Aug 25 2017, 10:07 AM

Aug 23 2017

MatzeB accepted D37085: [MachineOutliner] Add missed optimization remarks based off outliner cost model.

Looks straightforward to me. Though maybe wait for Adam to take a look as he has more experience in the area.

Aug 23 2017, 4:11 PM
MatzeB created D37083: LNT Make machine selection/update more flexible.
Aug 23 2017, 3:45 PM
MatzeB added a comment to D36847: [Support] Add reentrant start/stop Timer methods.

Replace Running member with StartCount.

Looking at https://reviews.llvm.org/D36946 I'm not convinced yet that timer is a good idea there at all.

Oh! Could you explain why you feel this way?

Aug 23 2017, 9:57 AM

Aug 22 2017

MatzeB added a comment to D36775: Increase tail dup threshold for -O3 from 3 to 4.

Pushed r311511 to stop taildup from unbundling instructions by accident. Post-commit reviews apreciated!

Aug 22 2017, 4:58 PM
MatzeB added a comment to D36415: Insert IMPLICIT_DEFS for undef uses in tail merging.

This should fix the problem without using stepForward: https://reviews.llvm.org/D37034

Aug 22 2017, 4:00 PM
MatzeB created D37034: Insert IMPLICIT_DEFS for undef uses in tail merging.
Aug 22 2017, 3:59 PM
MatzeB added inline comments to D36415: Insert IMPLICIT_DEFS for undef uses in tail merging.
Aug 22 2017, 2:16 PM
MatzeB added a comment to D36847: [Support] Add reentrant start/stop Timer methods.

FWIF: Looking at https://reviews.llvm.org/D36946 I'm not convinced yet that timer is a good idea there at all.

Aug 22 2017, 1:14 PM
MatzeB added inline comments to D36492: [time-report] Add preprocessor timer.
Aug 22 2017, 1:14 PM
MatzeB added a comment to D36866: [ARM] Add PostRAScheduling option.

Hi Matthias,

I agree that this would be nice, so I've had a play around it seems non-trivial to get a default configuration without breaking some tests. It is simple enough to predicate the PostRA scheduler on the availability of Thumb2 but this causes breakages for Swift. Do you know why we don't want to enable PostRA for MIScheduled arm cores? PostRA is used on almost all of the AArch64 cores along with the MIScheduler, so I'm wondering why the two approaches are different.

Aug 22 2017, 12:59 PM

Aug 21 2017

MatzeB added a comment to D36775: Increase tail dup threshold for -O3 from 3 to 4.

For the record: This broke the greendragon "Project Clang Stage 1: cmake, RA, with expensive checks enabled" build (possibly hidden by other errors).

Aug 21 2017, 6:33 PM
MatzeB added inline comments to D36866: [ARM] Add PostRAScheduling option.
Aug 21 2017, 1:55 PM
MatzeB added a comment to D36847: [Support] Add reentrant start/stop Timer methods.

For something as simple as a timer, having two std::string, 3 pointers and now a vtable feels like too much. Of course that's not the fault of this patch.

Aug 21 2017, 10:28 AM
MatzeB accepted D36768: [test-suite] Add -i option to fpcmp to ignore whitespace changes..

LGTM

Aug 21 2017, 9:35 AM

Aug 18 2017

MatzeB updated the summary of D36913: TableGen: Fix subreg composition/concatenation.
Aug 18 2017, 7:01 PM
MatzeB added a dependency for D36913: TableGen: Fix subreg composition/concatenation: D36911: TableGen: Add --gen-register-info-debug-dump.
Aug 18 2017, 7:00 PM
MatzeB added a dependent revision for D36911: TableGen: Add --gen-register-info-debug-dump: D36913: TableGen: Fix subreg composition/concatenation.
Aug 18 2017, 7:00 PM
MatzeB created D36913: TableGen: Fix subreg composition/concatenation.
Aug 18 2017, 6:59 PM
MatzeB created D36911: TableGen: Add --gen-register-info-debug-dump.
Aug 18 2017, 6:28 PM
MatzeB added a comment to D35598: Rework machine creation strategy.

Hi James,

Hi,

So I am not completely convinced the automatic machine name creation is a desirable behavior. I can see the convenience of machines getting created automatically at the cost of the machine names becoming less meaningful.

Having said all that I'd be fine to add a flag supporting a variation of the previous behavior where we create new machines if the machine data doesn't match (however I'd slightly change the behavior to append a number to the new machines name to maintain the property that machine names are unique). Would that be fine with you?

Before I go in to more detail about how we're testing (for background, and your interest) - that sounds like a very helpful solution, thanks!

this is very interesting to hear as I would not have expected the previous behavior to be desirable. Just to explain some more where I am coming from:

  • LNT Submissions are typically performed by CI jobs which for us are required to have a unique name, so it is natural to use the same LNT machine name as the CI jobs name.

This is also true for us, however we are building nightly using 30 groupings of "machines" (which are themselves pools of real machines driven by buildbot), and with historical data going back 4 years. Each of these groupings of machines use names automatically derived from the key aspects of their hardware and release branch they track, and we're diligent at recording more subtle machine differences in the "machine" field.

  • When selecting a machine in LNT the only thing to go on is the machine name. If for example I have 7 different machines named "gcc7" (with some of the other fields differing), I would need to click 7 times today, to figure out which is the machine that I want.

That is probably where the difference in perspectives comes from. We would have 7 machines producing results, which we expect to have identical configuration, and which we would group under the name "gcc7.$target_board.$target_cpu". In normal use, we would not expect 7 machines named "gcc7" to produce a result in one night, we would expect there to be one "active" gcc7 at a time (measured in months), and so choosing the right machine would be a matter of picking the one which has been building most recently. Occasionally due to sysadmin/user error, one of the machines in the pool might malfunction and end up in an inappropriate configuration. As an example from today, one rogue machine in the pool was accidentally patched up to a newer kernel version. When we import a run from that machine, the old LNT behaviour would create a separate machine, ensuring that the data integrity was maintained with the new machine isolated (which it wouldn't be if we forced --update-machine) but that we still had data in the system that we could compare (which we wouldn't get with the new error behaviour). This becomes important to us when importing historical data, as we really do want a new machine every time configuration changes, but we don't want to have to encode that in the machine name. Put another way, when we migrate a grouping of boards to a more recent kernel version, we want that change to make the data sets disjoint, but without us having to invent a new name for the machine pool.

Automatically appending a number to the machine name would therefore work well for our use case.

  • Similarily when connecting 3rd party visualisation/analysis to LNT it is convenient to have unique machine names that you can reference. Machine id numbers are only valid within one LNT database, and are also not predictable.

We don't do this, but I can see why this would be useful to you.

  • Looking at lnt runtest test-suite mode, nobody even bothered filling out the machine fields and to my knowledge nobody complained to this day.

We're a somewhat unique consumer of LNT, in that we have a completely separate infrastructure for running and recording test results, we generate JSON from this infrastructure which is suitable for import to LNT for visualisation. We make heavy use of the machine fields.

I'm very grateful for your help in resolving this, I appreciate we're running a non-standard configuration over here!

James

Aug 18 2017, 2:05 PM
MatzeB added a comment to D36683: [test-suite] Adding miniFE Benchmark.

Thanks for the review. I corrected the LICENSE.TXT formatting and removed the unneeded quotes.

Brian

Aug 18 2017, 1:58 PM
MatzeB added a comment to D36717: [test-suite] Add SPEC CPU 2017.

One more remark: The 'ref' dataset of 638.imagick_s on all the computers I tried on took between 2 and 3 hours. Submitted results to SPEC are in a similar range. Unfortunately timeit.py has a 7200 seconds (2 hours) limit hardcoded (for --limit-cpu and --timeout). I had to remove that limit to be able to have a successful run.

Can we make the limit configurable or set higher?

Let's make it configurable. There isn't any hard-coded value that will work well in all situations.

Aug 18 2017, 11:10 AM
MatzeB accepted D36680: [test-suite] Adding Pathfinder Benchmark.

LGTM

Aug 18 2017, 10:51 AM
MatzeB accepted D36683: [test-suite] Adding miniFE Benchmark.

LGTM (nitpicks below).

Aug 18 2017, 10:48 AM
MatzeB accepted D36682: [test-suite] Adding miniAMR Benchmark.

Thanks for the clarification on the license. LGTM.

Aug 18 2017, 10:44 AM

Aug 17 2017

MatzeB accepted D36768: [test-suite] Add -i option to fpcmp to ignore whitespace changes..

This is looking good! I am wondering if one of the if blocks is still necessary (see below).

Aug 17 2017, 4:33 PM
MatzeB accepted D36717: [test-suite] Add SPEC CPU 2017.

I'm still waiting to get my hands on the benchmark to try this out myself.

Aug 17 2017, 11:56 AM
MatzeB added a comment to D36768: [test-suite] Add -i option to fpcmp to ignore whitespace changes..
  • Adding a mode to ignore whitespace seems reasonable.
  • I remember seeing fpcmp loop endlessly and never got around figuring out why, good to see this fixed.
Aug 17 2017, 11:43 AM

Aug 16 2017

MatzeB added inline comments to D36721: [MachineOutliner] AArch64: Avoid saving + restoring LR if possible.
Aug 16 2017, 1:00 PM
MatzeB added inline comments to D36721: [MachineOutliner] AArch64: Avoid saving + restoring LR if possible.
Aug 16 2017, 12:49 PM

Aug 15 2017

MatzeB added a comment to D36680: [test-suite] Adding Pathfinder Benchmark.
  • Integration looks good to me
  • I'd like to hear some clarification on the license in the file headers
  • You could add a REAMDE file describing the benchmark in the benchmark directory
Aug 15 2017, 2:57 PM