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?

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.

Reading python docu and playing a bit with a prototype I believe this is the way to do things:

In D43168#1012849, @krytarowski wrote:

-pthreads defines additional OS specific flags like _REENTRANT or _PTHREADS and this is the form that should be used.

In D43107#1011163, @nruslan wrote:

@MatzeB : mstack-probe-size=0 is already used for a different purpose. It basically, if I remember correctly, always forces calls to chkstk regardless of the actual stack usage. Normally that would only happen after 4K. Regarding your second question -- do we really need to have these flags to be mutually-exclusive? They are related but serve a different purpose. stack-probe-size merely changes the default probe size from 4K to the specified value. mstack-arg-probe/mno-stack-arg-probe enables/disables stack probes. If we disable stack probes, their size no longer matters, but we do not necessarily have to prohibit stack-probe-size. So, in fact, I can imagine somebody may even define system-wide alias if they want to override 4K (for whatever reason). Then some Makefile (which is unaware of this alias) may, for example, disable stack probes completely (like in UEFI code case), and we do not necessarily want to prohibit this combination.

I put my handleMove() tests into unittests/MI/LiveIntervalTest.cpp, with manually crafted MI snippets and a specified move of a single instruction.

• Have you considered using stack-probe-size=0 instead?
• Doing it as a separate no-stack-arg-probe probably needs an IR verifier rule that forbids setting stack-probe-size and no-stack-arg-probe at the same time. Maybe also some attribute compatibility rules for inlining?

LGTM, with same change to avoid unnecessary repeated shrinkToUses().

In D43337#1009059, @thegameg wrote:

In D43337#1009040, @MatzeB wrote:

Printing different things depending on whether asserts are enabled feels strange. Can you explain the motivation here?

I think we should be showing all the information we have (redundant reg classes, redundant successors, predecessors (always redundant)) in things like -print-after-all, or when calling MF.dump() from the debugger (basically, when we're actually debugging llvm). Also, this can be too verbose to have when calling MF.print() for testing or other purposes.
I'm basically trying to come up with an implicit -simplify-mir for MF.print().

I guess if we don't really care about having MF.print() being too verbose, we could always assume IsStandalone is true except for MIR printing.

Printing different things depending on whether asserts are enabled feels strange. Can you explain the motivation here?

In D43165#1007792, @MatzeB wrote:

'Latin-1' seems completely arbitrary and wrong to me. You should rather try to get a stream of bytes rather than have python try to decode the file.

'Latin-1' seems completely arbitrary and wrong to me. You should rather try to get a stream of bytes rather than have python try to decode the file.

LGTM

In D43168#1005380, @dim wrote:

In D43168#1005331, @MatzeB wrote:

@asbirlea: Why is -dl and -pthread needed at all? Glancing at the code I don't see any reference to pthreads or dlopen/dlsym...

Hmm that is a good one, indeed! I have locally tried deleting the blocks that add -lpthread and -ldl, and at least on Linux, the programs in question all appear to link just fine.

On FreeBSD, I do get errors about pthread functions, though:

[789/4485] Linking CXX executable Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid
FAILED: Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid
: && /home/dim/obj/test-suite/build/tools/timeit --summary Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid.link.time /share/dim/llvm/324090-rel60-freebsd12-amd64-ninja-rel-1/bin/clang++  -O3 -DNDEBUG   Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/driver.cpp.o Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/__/common/x86_halide_runtime.bc.o Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/bilateral_grid.bc.o  -o Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid   && :
Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/__/common/x86_halide_runtime.bc.o: In function `halide_spawn_thread':
posix_allocator.cpp:(.text+0x3ab): undefined reference to `pthread_create'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[790/4485] Linking CXX executable Bitcode/Regression/vector_widen/widen_bug
FAILED: Bitcode/Regression/vector_widen/widen_bug
: && /home/dim/obj/test-suite/build/tools/timeit --summary Bitcode/Regression/vector_widen/widen_bug.link.time /share/dim/llvm/324090-rel60-freebsd12-amd64-ninja-rel-1/bin/clang++  -O3 -DNDEBUG   Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/driver.cpp.o Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/halide_runtime.bc.o Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/vector_widen.bc.o  -o Bitcode/Regression/vector_widen/widen_bug   && :
Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/halide_runtime.bc.o: In function `Halide::Runtime::Internal::default_do_par_for(void*, int (*)(void*, int, unsigned char*), int, int, unsigned char*)':
simd_op_check_runtime.ll:(.text+0x1dd): undefined reference to `pthread_create'
Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/halide_runtime.bc.o: In function `halide_spawn_thread':
simd_op_check_runtime.ll:(.text+0x651): undefined reference to `pthread_create'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[794/4485] Building CXX object Bitcode/Benchmarks/Halide/local_laplacian/CMakeFiles/halide_local_laplacian.dir/local_laplacian.bc.o
ninja: build stopped: subcommand failed.

Apparently there is a halide_spawn_thread function in the bitcode. I am unsure if there is any source for that bitcode?

@asbirlea: Why is -dl and -pthread needed at all? Glancing at the code I don't see any reference to pthreads or dlopen/dlsym...

In D43168#1005292, @krytarowski wrote:

For such options there are designed other options, like CMAKE_EXE_LINKER_FLAGS. target_link_libraries() should be used for libraries.

• No test.
• What about -mstack-arg-probe, shouldn't we have that for consistency?
• I'd prefer not to review clang changes myself as I don't know that part of the code too well.

The CMAKE_DL_LIBS change LGTM.

I agree that a function named printXXX() should not also clear the timer.
As far as I understand it this shouldn't change any behavior in the existing clang/llvm code, so LGTM.

In D42576#1003685, @dschuff wrote:

I'll just add that what we do in WebAssembly right now for unprototyped functions is to codegen them using the fixed-arg calling convention; at link-time if there are any mismatches between the definition and any calls, then the program has UB, and the resulting WebAssembly module will be invalid. I think this currently has the problem that if the definition does turn out to be vararg, then we have generated an invalid module even though the program is correct (I consider that a bug or shortcoming in our toolchain though). In any case, we do aim to support correct programs with prototypeless functions. If the test suite has non-vararg cases where there are mismatches between the callsites and/or the definition (I have seen that in some other test/benchmark suites), then even though that works on most X86 and ARM ABIs that I know of it's still UB and I'd be in favor of removing it from LLVM's test suite.

In D42576#1003045, @arichardson wrote:

Your example will work for x86 and probably ARM, but as far as I can tell it is up to the ABI which calling convention should be used for functions without prototypes.
For us these calling conventions are incompatible since the variadic one will pass arguments on the stack instead of in registers so if the target function is not actually variadic this will cause it to read garbage from undefined registers. While it *should* work if there is only zero or one arguments it is not guaranteed to work with more than 1.

I also found that apparently WebAssembly does the same: https://github.com/WebAssembly/tool-conventions/issues/16 and it also causes issues with functions without prototypes (https://bugs.llvm.org/show_bug.cgi?id=35385)

Just commit whatever version you see fit, I'm fine with any of the proposals; the bots will come back to you and tell you if things don't build somewhere :)

LGTM
...and except maybe for the renaming of the class this all looks like trivial changes that are fine with post-commit review.

In D42879#999811, @scanon wrote:

Underflow or overflow doesn't change sign, so 0 < C < inf && X >= 0 --> C/X >= 0.

In D42879#999308, @MatzeB wrote:

In D42879#999160, @spatel wrote:

Do we need to restrict this to 1.0 / X ? If we only care about the sign of the fdiv result and we're ruling out INF, then any constant 'C / X' should be ok? Can also handle 'X / C'?

It gets slightly harder with C/X because for large C we can underflow to zero if X is big enough. I wasn't sure how to compute the limit so went with 1.0 for now.

In D42879#999160, @spatel wrote:

Do we need to restrict this to 1.0 / X ? If we only care about the sign of the fdiv result and we're ruling out INF, then any constant 'C / X' should be ok? Can also handle 'X / C'?

Thanks for writing this! And LGTM in it's current form already.

Nice trick wmi, I tried it and could create a very small testcase that would hit the problematic code in question. Unfortunately those small inputs would not fail in -verify-machineinstrs mode without this patch (I suspect the liveranges didn't happen to actually be invalid even though we ignored the fact that PHIs can have inputs).

LGTM, thanks.

Thanks for doing all the testcase fixup work!

Argh:

...
    BX_RET 4, killed %cpsr, implicit %r0
    Successors according to CFG: %bb.2(0x40000000 / 0x80000000 = 50.00%)
...

A conditional return statement breaks the whole nice logic of MachineBasicBlock::isReturnBlock() returning true or false, since it's clearly neither in this case...

So do we still need all the .ll file changes with the syntax nof/mof syntax changes?

Actually can you rename the scheduling model to AArch64SchedExynosM3.td? (Feel free to do the same with the M1 file in a separate commit without review).

Of course reverting is fine. This whole commit was mostly motivated by llvm/test/CodeGen/AArch64/big-callframe.ll but you could simply XFAIL that test together with the revert.

In D42450#990855, @MaskRay wrote:

Is the decision "we should not make it Python 3" ?

In D42534#990357, @sdardis wrote:

+CC'ing @MatzeB for additional insight on what needs to be changed.

Found the bug. What's occurring with the debug information test failures with this patch, is that during the insertion of debug instructions for function arguments, it has to determine which register to assign to the debug instruction, see SelectionDAGISel.cpp:505. Here it asks for the frame pointer register which for some RISC like targets by the size of the frame for the function.

It seems to me that we need to compute the max call frame size before that point (SelectionDAGISel.cpp:505) for targets which use call frame setup and destroy pseudo instructions, this however requires changes for every target which implements finalizeLowering() to compute the maxCallFrameSize.

I can't say much about the scheduling model itself as I'm not familiar with Exynos.

I guess sooner or later we have to change this test to just check for the two cases of
-mattr=+zcz / -mattr=-zcz
and having a separate test that checks whether your favorite CPU is setting all the subtarget features you expect.

LGTM

I was contemplating whether we need a check that the register assigned on a renamable operand is a physreg. Of course we cannot even write a verifier for that as using MO::isRenamable() will immediately run into an assertion failure when that is not the case (which is good!).

In D42576#990020, @arichardson wrote:

Calling K&R functions is not a problem. It is fine to use them as long as there is a declaration with the parameter types in the LLVM IR and not just an declare i32 @foo(...). The problem is that we are calling functions that don't have a declaration and therefore the compiler assumes that it is a variadic function call. This leads to runtime crashes on our CHERI platform.

They are not necessarily correct C programs since they include undefined behaviour

Oh before, have the exact same problem in LiveRegUnits::addLiveOuts(). I can take a look at that later if you don't beat me to it.

As far as I can tell, those are correct C programs.

In D42411#989024, @sdardis wrote:

Anyway let's leave it as is, if it breaks the build on your side.

Just to double check, the 'it' you're referring to is the build system / compilation options for this test and you're happy with the patch as is?

This apparently defeats the logic in splitStores() (aka the infamous FeatureMisalignedSlow128Bit) because that code had an exception for v2i64 vector types but wants to split all other vector types. An internal test case of ours:

define void @test_split_16B(<4 x float> %val, <4 x float>* %addr) {
  store <4 x float> %val, <4 x float>* %addr, align 8
  ret void
}

would no longer show the store getting split.

Ok the subtle detail I missed before is that noreturn calls may still throw an exception/cause stack unwinding apparently. In that case we indeed need to prepare for CSR restoration.

In D42509#988208, @kparzysz wrote:

In D42509#987226, @MatzeB wrote:

A noreturn call doesn't need to have the callers callee saved registers restored.

Why not? The non-returning call doesn't come back to its call site, but it goes somewhere. From the point of view of some function foo, all of that activity may happen inside the call to bar (which would have further calls in it). Not preserving non-volatile registers could cause them to end up being clobbered across the call to bar.

In D42411#987999, @sdardis wrote:

Update as per @MatzeB 's comment, checking before commit showed it broke the build on OSX.

In D42450#987234, @bogner wrote:

In D42450#987039, @MatzeB wrote:

so I must've done something to be able to run these scripts on my machine. :)

Well installing any homebrew or macports python (which is often a dependency for other packages) will give you a python2 with the proper symlinks in place. It's usually people maintaining CI infrastructured that are bothered by macOS (or linux) default installations.

Mac's system python comes with a python2.7 symlink. Today you don't need to do anything for this script to work.

A noreturn call doesn't need to have the callers callee saved registers restored. I also think this fact shouldn't change with throwing calls (I assume the unwinding tables would contain information on how to restore the callee saves in this case?).

I'd like to see llvm moving away from legacy python2.7

That needs discussion on llvm-dev to make sure everyone is on the same page, GettingStarted.rst is updated etc. Forcing just a single script to python3 is not a good idea.

(+CC AMDGPU so they can veto if they don't like the syntax)

I guess a more C++y solution would be to #include <cmath> and using std::isnan() instead of using the C headers. But I guess this is fine for now.

LLVM's GettingStarted.html says "python >= 2.7" is fine for LLVM development.

I didn't do in-depth performance tests. In principle this just remove a few "nop" instructions so I didn't expect big changes. The change gives the scheduler a bit more freedom though as there is not instruction redefining SP anymore... Did you only get regressions and no improvements from this?