In D49353#1163951, @wmi wrote:

In D49353#1163840, @MatzeB wrote:

For the record: The check is based on a LiveInterval::size() which gives you the number of segments. So I assume what is "huge" here is the number of basic blocks?

One segment can span multiple basicblocks. I am not sure whether one basicblock can have multiple segments inside of it theoretically, but it is uncommon. So emperically large number of segments mean large number of basicblocks, then large number of edge bundle nodes and high hopfield neural network algorithm cost.

For the record: The check is based on a LiveInterval::size() which gives you the number of segments. So I assume what is "huge" here is the number of basic blocks?

I think fastregalloc/-O0 isn't really expecting all liveness flags to be complete (unfortunately hard to tell/not written down).

My LGTM still stands (in case you are waiting for another approval).

In D47999#1162130, @MatzeB wrote:

I still think we should rather try to not set TracksRegLiveness early in the codegen pipeline instead. Let me try how well that works...

I still think we should rather try to not set TracksRegLiveness early in the codegen pipeline instead. Let me try how well that works...

• Patches should be relative to the toplevel test-suite directory.
• 94k of reference output seems a lot and smells like you may end up in the anti-pattern of spending most of the benchmark time in printing instead of doing calculations.
• We currently tend to not add -ffast-math as part of the makefiles, but instead people choose it together with their other optimization flags.
• How long does this benchmark run? (We aim for 0.5-1s on typical PC hardware); Bonus points if the duration can be changed from the commandline (no expections for matching reference outputs for different input size though).
• srand()/rand() produce different output with different libc implementations!

or maybe prependImplicitDef()?

I would consider setDesc() a rare/exceptional operation. So I'm not sure I like an extra loop for the common addOperand() maybe we can rather provide an insertOperand() to accomodate the setDesc() scenario you describe?

I fear the covered check will hit us perf wise either way (+CC some people from our GPU team) and that we need to find a different solution long term (like the Targets announcing the saved registers themselfes instead of the generic code piecing together the information...).
Anyway if this works for you I'd be fine with this patch as a stopgap.

Adding more people; still reading up on why the covered logic was necessary in the first place...

turns out my checkout was slightly outdated too, rebased. Fixed warning.

Makes sense, thanks for the patch.

Put an updated version of the patch here: https://reviews.llvm.org/D49256

In D37582#1160356, @rinon wrote:

In D37582#1160045, @thegameg wrote:

Thanks @rinon for this. Are you still interested in committing this? I would like to get this in to fix some X86 machine verifier issues.

Yep! I'll rebase and take a look at your MFI comments now, unless @MatzeB is already doing that.

Sorry this fell to the cracks. I tried comitting it yesterday, but it needs some adaptation for GlobalISel now. I'm currently making some simple changes for that and will then commit...

See also: https://reviews.llvm.org/D37582
(Looks like I never got around comitting it and nobody pinged, oops)

In D48674#1149645, @dblaikie wrote:

Out of curiosity, what's the use case/motivation for this?

Landed in rL335877

update

I don't have a copy of Spec2017 yet so I cannot test any of it.

LGTM

Looking around the code I would guess it just works. You should however also remove the && MRI.tracksLiveness() from MachineBasicBlock::print() if you change the default.

IMO MachineFunction::init() should not set TracksLiveness by default and instead VirtRegMap / FastRegAlloc should set it once it actually filled in the basic block live-in lists.

From what I remember our use of this flag is strange:

LGTM with this round of nitpicks addressed.

Take a look at RAGreedy::selectOrSplit for an example on how to report fatal errors in the backend.

In D48109#1131367, @thegameg wrote:

@chandlerc @jlebar can you confirm that the new PM doesn't have timer support?

I'm pretty sure I only added the timer->statistics support for the "legacy" pass manager.

usr/bin/env is the recommended way to invoke python... Or is lldb somehow hardcoded to use /usr/bin/python and you need to match this here?

I'd also recommend using the api_auth_token. Actually if you look at the Rest API you can see it supporting submissions only with api_auth_token set. I just never wanted to introduce this to the /submitRun pages to remain compatible with old clients.

I assume you don't have commit access?
Usually it's the author of the patch that commits it unless he doesn't have commit access yet in which case it's upon the reviewers.

LGTM

Why not let the database do the sorting? .order_by(ts.Machine.name) or whatever sqlalchemy wants there?

Cross compiling currently is a mess. Basically cmake only works properly when you have a complete gcc style cross toolchain, or a MacOS style toolchain on Mac. Most llvm developers currently are not in a position to set one of those two things up when they just want to test a compiler they made some changes to; to be honest when I started hacking on the llvm test-suite cmake I wasn't even aware of what assumptions are made there/what problem exist...

LGTM. Please watch the buildbots carefully (and revert in case of problems) when you push this.

No feedback from my side, I#m not familiar with the profile code.

(FYI: I'm busy with non-llvm at the moment, so I may not be responsive for things I cannot answer right away; but I'll try to find a timeslot this week going through the backlog of llvm things I have to look at)

In D46735#1096782, @hfinkel wrote:

In D46735#1096703, @Meinersbur wrote:

In D46735#1096128, @jdoerfert wrote:

First, let me keep the record straight:

• Only SingleSource/Benchmarks/Polybench profits from the (mostly tiling) transformations applied by Polly.
• There are various reasons why other benchmarks are "not optimizable" by Polly but only a fraction is caused by manual "pre-optimizations" (except the input language choice obviously).
• Adding simple "Polly-optimizable" benchmarks is all good and well (as it makes for nicer evaluation sections in future papers...), but I would argue it is much more interesting to investigate if the existing benchmarks could be optimized and why they currently are not.

I agree that studying existing sources, why they are not optimized, and improve the optimizer such that the reason is not an obstacle anymore, is the primary goal.
The problem is that this not feasible for all sources. For instance, the array-to-pointers-to-arrays style (which @proton unfortunately also used here; I call then "jagged arrays", although not necessarily jagged) cannot be optimized because the pointers may overlap. Either the frontend language has to ensure that this never happens, or each pointer has to be compared pairwise for aliasing. The first is not the case in C++ (without extensions such as the restrict keyword), the latter involves a super-constant overhead. Unfortunately, very many benchmarks use jagged arrays.
Second, even if it is possible to remove an optimization obstacle, I would like to know whether it is worth it.
Third, researchers in the field of polyhedral optimization work on improving the optimizer algorithm and ignore language-level details (e.g. whether a jagged, row-major or column-major arrays are used)

Regarding these (and other new) benchmarks:

• Please describe why/how the codes differ from existing ones we have (e.g., Halide/blur). Polybench already contains various kernels including many almost identical ones.

The Halide benchmarks are special in many regards; for instance, works only on x86.

• Please describe why/how the magic constants (aka sizes) are chosen. "#define windows 10" is not necessarily helpful.

At some point a problem size has to be arbitrarily defined. What kind of explanation do you expect?

• I fail to see how Polly is going to optimize this code (in a way that is general enough for real codes). So my question is: Did you choose a linked data structure on purpose or do you actually want to have a multi-dimensional array?

+1

In D46735#1096497, @hfinkel wrote:

In D46735#1096464, @proton wrote:

In D46735#1095482, @MatzeB wrote:

Are you writing these from scratch? If so, I'd like to make some suggestions:

• Please aim for a runtime for 0.5-1 second on typical hardware. Shorter benchmarks tend to be hard to time correctly, running longer doesn't increase precision in our experience.

But the fraction of noise will be more for shorter runtimes. A longer runtime will help us when we to see the performance improvement after applying optimization.

The question is: At what point is a performance change interesting? If we posit that a performance change is interesting at the ~1% level, and we can distinguish application-time running-time differences at around 0.01s, then running for 1-2s is sufficient for tracking. As the test suite gets larger, we have an overarching goal of keeping the overall execution time in check (in part, so we can run it more often). It's often better to collect statistics over multiple runs, compared to a single longer run, regardless.

Also, if there are particular kernels you're trying to benchmark it's better to time them separately. We have a nice infrastructure to do that now, making use of the Google benchmark library, in the MicroBenchmarks subdirectory.

IMHO we also want to see effects on workingset sizes larger than the last-level-cache. A micro-benchmark is great for small workingsets, but I am not sure whether Google's benchmark library works well with longer ones.

I don't see why it wouldn't work on longer-running kernels. Nevertheless, modern machines have bandwidths in the GB/s range, so a 1s running time is certainly long enough to move around a working set larger than your cache size.

Some optimizations (e.g. cache-locality, parallelization) can cut the execution time by order by magnitudes. With gemm, I have seen single-thread speed-ups of 34x. With parallelization, it will be even more. If the execution time without optimization is one second, it will be too short with optimization, especially with parallelization and accelerator-offloading which adds invocation overheads.

There are two difficult issues here. First, running with multiple threads puts you in a different regime for several reasons, and often one that really needs to be tested separately (because of different bandwidth constraints, different effects of prefetching, effects from using multiple hardware threads, and so on). We don't currently have an infrastructure for testing threaded code (although we probably should).

Second, I don't think that we can have a set of problem sizes that can stay the same across 40x performance improvements. If the compiler starts doing that, we'll need to change the test somehow. If we make the test long enough that, once 40x faster, it will have a reasonable running time, then until then, the test suite will be unreasonably slow for continuous integration. I think that we need to pick problems that works reasonably now, and when the compiler improves, we'd need to change the test. One of the reasons that I like the Google bechmark library is that it dynamically adjusts the number of iterations, thus essentially changing this for us as needed.

Some optimizations (e.g. cache-locality, parallelization) can cut the execution time by order by magnitudes. With gemm, I have seen single-thread speed-ups of 34x. With parallelization, it will be even more. If the execution time without optimization is one second, it will be too short with optimization, especially with parallelization and accelerator-offloading which adds invocation overheads.

It's great to have a discussion on how such benchmarks should look like.

Instead of one-size-fits-it-all, should we have multiple problem sizes? There is already SMALL_DATASET, which is smaller than the default, but what about larger ones? SPEC has "test" (should execute everything at least once, great to check correctness), "train" (for PGO-training), "ref" (the scored benchmark input; in CPU 2017 runs up to 2 hrs). Polybench has MINI_DATASET to EXTRALARGE_DATASET which are defined by workingset-size, instead of purpose or runtime.

• First: We have to choose a default problem size and I just wanted to emphasize that it should not be too big, so running the llvm test-suite finishes in a reasonable timeframe.

Are you writing these from scratch? If so, I'd like to make some suggestions:

It's odd to have this in the repository, but admittedly we don't really have a wiki or similar in LLVM so I may be ok.

(this patch is still good to go/accepted)

BTW: I did not do this years ago, because the doxygen documentation claims that the brief comment stops at the first linebreak or the first dot. Hence I was preaching the rule that you still need \brief when the comment is more than 1 line. I now heard that apparently this is not how doxygen behaves (so documentation bug I guess).

LGTM
Tgiugh this is another sign that the global_status view probably isn‘t used byanyone ans has no tests. I still think we should just remove it...

LGTM

I think this is fine, but maybe wait a week or so before comitting in case @cmatthews has more comments.

This seems straightforward.

Maybe the answer here would be to further break up templates/reporting/runs.html into smaller pieces that can then be included from the WebUI and the E-Mail reports. So we can leave the accordions in the web-ui part. (That may be a bunch of refactoring though)

templates/reporting/runs.html reporting is also used to produce E-Mail reports I believe. And I'm not sure the E-Mail output has the CSS required for the accordions?

Looks good enought to commit, though I would recommend factoring things out differently:

Oh sorry, I didn't notice there is another "scavenging" thing in there.

Huh? You cannot just disable stack slot scavenging. Who guarantees you that you don't need it?
Is the bug related to the latest changes in PEI? (I've seen some commits talking about base and frame pointers flying by but did not have time to read them or dive into the details...)

In D45708#1070667, @mattd wrote:

I diffed the two outputs and also pulled up the text in a browser. I do not see any significant changes in browser, and there wasn't. However, I am happy to revert if any noticeable changes are witnessed. I see multiline/paragraph text in the output.

• Do non gold linkers all accept the -Wl,-plugin-opt lines?
• Maybe the clang driver knows if a gold linker is used and could add the -Wl,-plugin opt lines itself when -save-stats is used?

I am pretty sure this is not correct: Autobrief is stupid and stops at the first dot or at the line ending AFAIK. This means when the first sentence stretches over more than 1 line you still need need \brief. So a bunch of the changes here appear invalid to me, please check the doxygen output and revert if necessary.

In D45695#1069868, @junbuml wrote:

Have you seen LiveRegUnits::accumulate() glancing over the code here (and earlier) it seems to me that TII::tracksRegDefsUses() was just invented to do the same thing: figuring out which registers are free/usable over a range of instructions...

Yes, actually my current change in TII::tracksRegDefsUses() was inspired by LiveRegUnits::accumulate(). The only difference is that we need to track UsedRegs and DefReg separately in TII::tracksRegDefsUses(), while LiveRegUnits::accumulate() track both used/defed registers together. Based on your comment in https://reviews.llvm.org/D41463#inline-400109, would it make sense to introduce new static member function in LiveRegUnits like :

static void LiveRegUnits::accumulateUseDef(MachineInstr &MI, LiveRegUnits &ModifiedRegUnits, LiveRegUnits &UsedRegUnits)  ?

Have you seen LiveRegUnits::accumulate() glancing over the code here (and earlier) it seems to me that TII::tracksRegDefsUses() was just invented to do the same thing: figuring out which registers are free/usable over a range of instructions...

or

$ clang -arch arm64 -O3 -S -o- t.c -mcpu=generic
...
	ldrb		w8, [x1]
	ldrb	w9, [x1, #1]
	ldrb	w10, [x1, #2]
	ldrb	w11, [x1, #3]
	strb		w8, [x0]
	strb	w9, [x0, #1]
	strb	w10, [x0, #2]
	strb	w11, [x0, #3]
	ret

Should we maybe move the requirement list into setup.py instead of using this functionality which they apparently don't want us to use like this? (I'm fine with this patch in the meantime)

LGTM, though none of us apple people know much about the profile code as it only works on linux right now. But unless someone with more knowledge of the code objects, I'd push this.

In D43314#1031716, @homerdin wrote:

In D43314#1025162, @rengolin wrote:

In D43314#1021294, @homerdin wrote:

Tried some things and was able to get it working using /dev/stdout.

That doesn't work on Windows, may not work on Mac?

Should I change it back to rm %t.results.out after FileCheck? I'm not sure how to do this with a pipe.

Thanks for working on this; I'm looking forward to have this!

This is great! I always wanted to see a IACTA like tool for other architectures.

... looks like we should add some tests for the stuff in profile_views.py

This is surprising: DBG_VALUE should not have any defs so removeDefs() should have no effect and all uses should be marked with the debug flag so none of them should pass the MachineOperand::readsReg() test in addUses. Could you check whether the debug flag is set correctly? Did you try running your test with -verify-machineinstrs?

Now, should we go to such length to make the debug information be friendlier to some debugger, I am not sure, but I let you decide.

Definitely not! That debugger constraint feels like it is designed for ancient compilers producing code and register allocating for one C statement at a time.

I've been looking into this, and we do indeed produce something like this for naked function with a parameter (this from http://llvm.org/PR28641 compiled with -O0):

bb.0 (%ir-block.1):
  liveins: $edi
  %0:gr32 = COPY $edi
  %1:gr32 = COPY killed %0
  INLINEASM &ret [sideeffect] [attdialect], $0:[clobber], implicit-def early-clobber $rdi, $1:[clobber], implicit-def early-clobber $eflags, !2

Looks good from what I can see, though I'm not super experienced with fast-isel either. Does this work correctly in the case when fast-isel aborts while selecting a block?

Looks nearly good to me. I'd just opt for a different naming of the sub-tests. Also some nitpicks/comments on other nitpicks below:

LGTM

• You should double check whether you really need to repeat the lit.local.cfg changes in each directory.
• I haven't actually tried compiling/running the benchmark but the changes LGTM.

This should be a good thing! Among other things it should also allow us to switch register mask operands from physical registers to register units.

Your reasoning makes sense to me. And I think it's fine to push this given no floating point expert objects.

• I'd rather keep MachineRegisterInfo to things directly related to information about virtual registers (register class etc.); I don't think it's role should be to provide transformations for involving virtual registers. So I'd rather move the functionality somewhere else.
• I'm skeptical this works as intended in the presence of control flow (= multiple uses and defs of a vreg anywhere in the control flow graph).
• I'm not sure I completely understand what is going on here, is it looking for COPYs killing a value so it can replace the debug values with the copied value? The commit message should mention that then.

This is a tricky case, good catch.

I'm still a bit confused as to why we start spilling unused input vregs in the first place. Can you add a test case so we can reproduce? (The test can't get commit without a test anyway)

• Just a warning: I'm a bit skeptical timers will work reliable for things that happen in well under a millisecond (did you do some sanity checking? i.e. do all the timer roughly add up to the time spent in the frontend?)
• In the same vain I wonder if you add overhead by adding timers to functions that are likely called hundreds of times for a typical source file. Have you measured a release build with and without your patch.

LGTM with declarations moved into a header.

Please append a "; NFC" to the commit title.
LGTM.

Okay reading the code a bit more seems like that is what you are doing.

But we don't want user flags for bugpoint, we want it to try both variants and keep the smaller one when it still reproduces the crash.

Since a "naked" function can only have nothing but "asm" statements inside, and those "asm" statements can not have input or output parameters, it is safe to just disable any registers spilling for a "naked" function.