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

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?

In D37562#877209, @craig.topper wrote:

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

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

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

Add a unittest to ensure we are indeed not 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.

LGTM

In D37421#875900, @MatzeB wrote:

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

In D37421#876105, @hfinkel wrote:

In D37421#875900, @MatzeB wrote:

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?

In D37421#875879, @hfinkel wrote:

In D37421#874416, @MatzeB wrote:

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

In D35396#875872, @Lekensteyn wrote:

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 {} ';'

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

In D37611#870745, @kparzysz wrote:

In D37611#870742, @JesperAntonsson wrote:

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.

LGTM

In D37698#869935, @rampitec wrote:

In D37698#869839, @MatzeB wrote:

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

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

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

LGTM with nitpicks.

In D37611#869555, @kparzysz wrote:

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.

In D37611#869543, @kparzysz wrote:

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

LGTM with nitpicks.

This is a very good idea!

In D33650#864758, @jonpa wrote:

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.

LGTM, thanks

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

In D33960#864222, @chandlerc wrote:

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.

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!

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

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

In D33960#862837, @dblaikie wrote:

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)

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

In D37083#860435, @jgreenhalgh wrote:

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.

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

LGTM.

In D37360#858537, @kristof.beyls wrote:

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.

Thanks for the review!

• MultiSource/Benchmarks/DOE-ProxyApps-C/SimpleMOC/LICENSE doesn't need to have the executable flag set
• Otherwise integration LGTM.

Integration LGTM.

This fails verification for me on macOS:

• No Makefile integration yet (though if this gets moved MicroBenchmarks, we cannot use Makefiles anyway, so this may be moot).

Integration LGTM.

Integration LGTM.

In D37034#858114, @kparzysz wrote:

Are there any new developments?

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

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.

In D34581#856259, @kristof.beyls wrote:

In D34581#856055, @MatzeB wrote:

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.

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.

LGTM

LGTM

In D37164#854194, @MatzeB wrote:

In D37164#854053, @gberry wrote:

In D37164#853040, @efriedma wrote:

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

In D37164#854053, @gberry wrote:

In D37164#853040, @efriedma wrote:

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?

In D36911#852699, @MatzeB wrote:

In D36911#852693, @qcolombet wrote:

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.

In D36911#852693, @qcolombet wrote:

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

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

LGTM

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

In D36847#850238, @modocache wrote:

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?

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

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

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

In D36866#848488, @samparker wrote:

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.

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

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.

LGTM

In D35598#840451, @jgreenhalgh wrote:

In D35598#839397, @MatzeB wrote:

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

In D36683#845804, @homerdin wrote:

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

Brian

In D36717#845089, @kristof.beyls wrote:

In D36717#844930, @Meinersbur wrote:

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.

LGTM

LGTM (nitpicks below).

Thanks for the clarification on the license. LGTM.

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

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

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

• 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