code change LGTM.
I'd be great if you can find a test for this. (Just in case the AMDGPU based test doesn't work out I'd be fine without one here, given how rarely computeRegisterLiveness() is used)

I'm convinced now the clobbering works correctly because as clobberRegister does not just clobber the register at hand but the whole destination register of the copy touched by the COPY at hand.
LGTM (this time with the right review :)

I don't really know how exception handling on windows works or what a funclet is. So here's just a couple style comments and questions

I'm convinced now the clobbering works correctly because as clobberRegister does not just clobber the register at hand but the whole destination register of the copy touched by the COPY at hand.
LGTM

LGTM

The new .mir test is nicely concise now. Maybe we can even do without the .ll test now?

LGTM

LGTM
We may consider adding something like a tooltip in the future with the fully expanded value...

LGTM

LGTM with nitpicks addressed.

Landed in rL342589

Landed in rL342590

Address review feedback.

Please create a .mir test that just checks the ARMLoadStoreOptimizer pass. See test/CodeGen/ARM/vldm-liveness.mir or test/CodeGen/ARM/load_store_opt_kill.mir for example.

Isn't this fix about inline assembly? Why do we see all the scheduling/regalloc changes here?

Isn't this fix about inline assembly? Why do we see all the scheduling/regalloc changes here?

Some additional coding style comments

I've seen callbacks lead to unintuitive/hard to understand code. Can you motivate the use case some more?

In D52010#1233619, @qcolombet wrote:

Hi Matthias,

Thanks for modernizing this allocator.

Could you split the patch to make the review easier?

You mentioned several points in your description, if at all possible, it would be nice to have one patch for each.

Cheers,
-Quentin

I can try to do that for review purposes... But some things depend on each other so the intermediate steps are probably not going to be functional...

Out of interest: What version of pycodestyle do you use? I currently seem to have version 2.4.0 here while I do see most of the issues, for example I don't see a message about the regex functions not using raw strings.

LGTM once Eli approves.

Looks obvious and LGTM.

In D51752#1226462, @MatzeB wrote:

• I assume the review lacks the changes on the llvm side?

• I'm not a good person to review clang changes.
• I assume the review lacks the changes on the llvm side?
• On the LLVM side this feels like something for Analysis/ValueTracking.h in fact I already see Value *isBytewiseValue(Value *V); there, maybe that is good enough for your purpose?

Nice change, LGTM!
Maybe wait a couple days before committing in case someone better versed with SelectionDAG wants to chime in.

LGTM, sorry for the breakage.

In D51048#1223622, @tra wrote:

In D51048#1221432, @MatzeB wrote:

Though thinking about it, I don't really understand your problem:

How can there be multiple outputs with the same name? They have different names in the repository (algorithm.reference_output, assert.reference_output, ...)

For CUDA tests, from each test source file we generate a lot of test executables compiled for combination of {CUDA version, C++ standard, C++ standard library}. All of those test executables will run and need to have their output compared to the same reference file.

In D51048#1221431, @MatzeB wrote:

In D51048#1221423, @tra wrote:

I've ran into an unexpected problem after this patches have landed.

CUDA test suite generates multiple tests that use the same reference output.
I can't just use llvm_test_data(test_XYZ, "test.reference_output"), because there can only be one test.reference_output in the build directory.
I also can't manually create a symlink $SRC/test.reference_output -> $BUILD_DIR/test-XYZ.reference_output and then use llvm_test_data(SOURCE_DIR=$BUILD_DIR test-XYZ.reference_output) because it can't link to itself.
The only way around is to create a new directory, symlink original file there as test_XYZ and then use the temp dir as SOURCE_DIR to create test_XYZ.reference_output in the build dir. This is rather convoluted.

We probably need llvm_test_data with an optional argument that would allow providing a different name for the file name in the build dir, or make it unique automatically.

Possible fix for your case (untested as I don't have a CUDA environment):

diff --git a/External/CUDA/CMakeLists.txt b/External/CUDA/CMakeLists.txt
index 071ffbc9..63ab1e4c 100644
--- a/External/CUDA/CMakeLists.txt
+++ b/External/CUDA/CMakeLists.txt
@@ -58,13 +58,16 @@ macro(create_one_local_test_f Name FileGlob FilterRegex)
     set(_executable ${Name}-${VariantSuffix})
     set(_executable_path ${CMAKE_CURRENT_BINARY_DIR}/${_executable})
     # Verify reference output if it exists.
+    llvm_test_run()
+    set(REFERENCE_OUTPUT)
     if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${Name}.reference_output)
-      llvm_test_traditional(${Name})
-    else()
-      # otherwise just run the executable.
-      llvm_test_run()
+      set(REFERENCE_OUTPUT ${Name}.reference_output)
+      llvm_test_verify(WORKDIR ${CMAKE_CURRENT_BINARY_DIR}
+        ${FPCMP} %o ${REFERENCE_OUTPUT}
+      )
     endif()
     llvm_test_executable(${_executable} ${_sources})
+    llvm_test_data(${_executable} ${REFERENCE_OUTPUT})
     target_compile_options(${_executable} PUBLIC ${VariantCPPFLAGS})
     if(VariantLibs)
       target_link_libraries(${_executable} ${VariantLibs})

In D51048#1221423, @tra wrote:

I've ran into an unexpected problem after this patches have landed.

CUDA test suite generates multiple tests that use the same reference output.
I can't just use llvm_test_data(test_XYZ, "test.reference_output"), because there can only be one test.reference_output in the build directory.
I also can't manually create a symlink $SRC/test.reference_output -> $BUILD_DIR/test-XYZ.reference_output and then use llvm_test_data(SOURCE_DIR=$BUILD_DIR test-XYZ.reference_output) because it can't link to itself.
The only way around is to create a new directory, symlink original file there as test_XYZ and then use the temp dir as SOURCE_DIR to create test_XYZ.reference_output in the build dir. This is rather convoluted.

We probably need llvm_test_data with an optional argument that would allow providing a different name for the file name in the build dir, or make it unique automatically.

Good idea! LGTM.

• Address remaining review feedback, shorten some terminal dumps.
• I will commit this after landing D51080 (the remote section here describes the situation after that patch)

I currently see on my machine:

[0/1] Running the LLVM regression tests
llvm-lit: /Users/mbraun/dev/public_llvm/utils/lit/lit/TestingConfig.py:102: fatal: unable to parse config file '/Users/mbraun/dev/public_llvm/build/test/lit.site.cfg.py', traceback: Traceback (most recent call last):
  File "/Users/mbraun/dev/public_llvm/utils/lit/lit/TestingConfig.py", line 89, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/Users/mbraun/dev/public_llvm/build/test/lit.site.cfg.py", line 59, in <module>
    lit.llvm.initialize(lit_config, config)
AttributeError: 'module' object has no attribute 'llvm'

Good catch, the fix looks good to me. I assume you built llvm will all (non-experimental) targets and tested them?

And for some context about the seemingly excessive use of WORKDIR ${CMAKE_CURRENT_BINARY_DIR} here: One of my original plans was to make the test-suite build directory relocatable, i.e. after building you could move it to any other directory and still correctly run the benchmarks. That would mean have to stop using absolute paths and need to express everything relative to the benchmark binary. I hoped that an intermediate step towards this is changing as much as possible to WORKDIR ${CMAKE_CURRENT_BINARY_DIR} + relative paths. A next step would then be to change the default workdir to the binary dir.

In D51048#1219748, @proton wrote:

After applying this patch, the tests fail for me because it is taking the hash of stdout instead of the file mentioned in llvm_test_verify_hash_program_output macro.
Changing line 64 of cmake/modules/SingleMultiSource.cmake to ${CMAKE_SOURCE_DIR}/HashProgramOutput.sh ${file} will fix this.

Yes sorry, I had fixed this in my local copy too but failed to update the review.

Thanks for the detailed review comments!

Thanks for looking into this!

LGTM too; some nitpicks below.

In D51080#1217564, @Meinersbur wrote:

In D51080#1216896, @MatzeB wrote:

ping

The summary mentions this is for early review. Has this changed?

Ah yes, the first versions of this patch was not working in some corner cases like PGO. But with the latest updates I consider this patch ready.

The new report and its implementation look good to me! However before committing this needs at least a basic unit test (tests/server/ui/V4Pages.py) to check that the page renders without exception and produces valid html.

This change LGTM and should be committed.

LGTM

LGTM, thanks

ping

ping

In D49097#1213446, @tpr wrote:

This fix adds code to scan all uses of a subrange and extend it, after every join. I can't think of a way of doing it in a lower impact way.

Which has got me thinking:

Am I right about all of the following?

1. The decision on whether to join a copy is taken only with reference to each register's main range, not the subrange.

Yes.

2. The only reason it does similar stuff with conflict detection and resolution on the subranges is so it can modify them incrementally.

Yes, it's mostly about updating the subranges.

3. This fix, scanning all uses and extending subranges after a join, pretty much nullifies the advantage of modifying subranges incrementally.

Two reasons:

1. Updating the liveranges was considered better for compiletime (I mean you could remove even the non-subrange updating logic with the same reasoning to make the code even simpler...)
2. Recalculating subregister liveness used to not work reliably after coalescing (There were corner cases with full register uses with only some lanes being defined, without having corresponding IMPLICIT_DEF instructions for the undefined lanes).

LGTM

• Fix PGO in remote setting: We have to do the merging on the host side where the compiler toolchain resides, however before we can do that we have to retrieve the profile data from device. Make the remote module add commands to retrieve the profile data.

• Address review comments
• Execute METRIC: lines remotely

In D51080#1210083, @Meinersbur wrote:

In D51080#1209955, @MatzeB wrote:

In D51080#1209934, @Meinersbur wrote:

1. cmake builddir for cross-compilation
2. make/ninja
3. rsync pushes test-suite builddir to target
4. llvm-lit locale, but run every command on remote using ssh
5. including fpmc/VALIDATE step in *.test (SPEC2017 also uses llvm_add_host_executable)

The VERIFY: steps in the scripts are run remotely as well.

PREPARE: and METRIC as well?

Yes PREPARE: is modified as well, and I just realized METRIC: should be modified too but is still TODO :)

In D51080#1209934, @Meinersbur wrote:

Can you outline the intended workflow? My guess:

1. cmake builddir for cross-compilation
2. make/ninja
3. rsync pushes test-suite builddir to target
4. llvm-lit locale, but run every command on remote using
5. including fpmc/VALIDATE step in *.test (SPEC2017 also uses llvm_add_host_executable)

The VERIFY: steps in the scripts are run remotely as well. I hope that takes part of the spec2017 validation things[*]

LGTM with nitpicks addressed.

I am ok with committing this, but maybe we should have someone else's opinion as well?

Updated patch:

This looks wrong to me. And it would be good if you could explain with more detail or an example. So far I see: An instruction uses a register but we didn't add a register operand for that. Why would we fix this by adding an exception into shrink wrapping instead of making sure there is an actual operand?

In D50988#1206371, @kbarton wrote:

In D50988#1206363, @MatzeB wrote:

Actually can you explain the removal of llvm/test/tools/dsymutil/PowerPC/sibling.test?

I had an email conversation with @JDevlieghere regarding this test. Someone suggested a test based on an old version of llvm-gcc from an old Powerbook G4, which is how this test was created. That was the only reason for this being a PowerPC test.

I tried generating an alternate test using a new GCC on a ppc64le machine, but the test caused an Assert failure in dsymutil. Jonas is looking at. In the meantime, he said this specific test should not block this patch.

If you have another idea for generating an equivalent test, I'm happy to do it.

Actually can you explain the removal of llvm/test/tools/dsymutil/PowerPC/sibling.test?

I just realized that this is particular test is only about making the tests not use a darwin target anymore. That LGTM.

I personally can't think of a reason why not to do this. But I think sending an RFC to llvm-dev is in order here.

LGTM
Just change the title when committing since this is in generic code and will affect every target and not just "WebAssembly"

In D50339#1206109, @kparzysz wrote:

The practical application of this is to indicate aliasing with a given memory object, but without specifying exactly which part of the object is accessed (since it may be unknown). Dropping memory operands altogether will alias the operation with all other objects, so it will be correct, but overly conservative.

In D49401#1177039, @arsenm wrote:

In D49401#1177010, @MatzeB wrote:

Also I'd generally recommend to rather look at LivePhysReg/LiveRegUnits for doing liveness queries post-ra. They can be more expensive but give more consistent results...

I want to use this in an SSA pass, not post-RA

In D49671#1173217, @jonpa wrote:

In D49671#1172527, @MatzeB wrote:

This is tricky. Some comments:

• Have you tried enabling subregister liveness tracking? Among other things it gets rid of the implicit-defs/uses for the full registers... (though there may be other factors influencing that decisions)

Yes, IIRC I have tried that, but got crashes immediately which was discouraging. So for the moment, that is not something that could be the default for SystemZ, I think.

• What about just setting the latencies induced by the artifical implicit def-/uses[1] to 0? [1] = in lack of a better way to identify them, that would be all implicit vreg defs/uses that are not part of the MCInstrDesc.

Yes, that was also my idea but as I wrote earlier in some rare cases I noticed instructions where the actual latency was only put on that extra regalloc operand, while the explicit use op had just a unit latency!

I looked into this now a bit more, and it seems that in these cases a multiply or other instruction requires a double word register (128 bit), so a 64 bit register is coalesced into it:

Before Coalescing:

16B       %0:gr64bit = LGFRL @seedi ::
128B      undef %5.subreg_l64:gr128bit = COPY %0:gr64bit
144B      %6:gr128bit = COPY %5:gr128bit
160B      %6:gr128bit = MLGR %6:gr128bit, %3:gr64bit

After Coalescing:

16B       undef %5.subreg_l64:gr128bit = LGFRL @seedi :: (dereferenceable load 4 from @seedi)
...
144B      %6:gr128bit = COPY %5:gr128bit

After RA:

bb.0.entry:
renamable $r1d = LGFRL @seedi, implicit-def $r0q :: (dereferenceable load 4 from @seedi)
...
renamable $r4q = COPY renamable $r0q

After Post-RA pseudo instruction expansion pass:

bb.0.entry:
renamable $r1d = LGFRL @seedi, implicit-def $r0q :: (dereferenceable load 4 from @seedi)
...
$r4d = LGR $r0d, implicit $r0q
$r5d = LGR $r1d, implicit $r0q

DAG has the latency on $r0q (superreg), instead of $r0d between SU(0) and SU(3). ($r0q = $r0d + $r1d):

SU(0):   renamable $r1d = LGFRL @seedi, implicit-def $r0q :: (dereferenceable load 4 from @seedi)
# preds left       : 0
# succs left       : 9
# rdefs left       : 0
Latency            : 5
Depth              : 0
Height             : 70
Successors:
SU(4): Data Latency=5 Reg=$r1d
SU(4): Data Latency=5 Reg=$r0q
SU(3): Data Latency=5 Reg=$r0q
SU(3): Data Latency=1 Reg=$r0d
...
SU(3):   $r4d = LGR $r0d, implicit $r0q
# preds left       : 2
# succs left       : 3
# rdefs left       : 0
Latency            : 1
Depth              : 5
Height             : 65
Predecessors:
SU(0): Data Latency=5 Reg=$r0q
SU(0): Data Latency=1 Reg=$r0d
...
SU(4):   $r5d = LGR $r1d, implicit $r0q
# preds left       : 2
# succs left       : 3
# rdefs left       : 0
Latency            : 1
Depth              : 5
Height             : 65
Predecessors:
SU(0): Data Latency=5 Reg=$r1d
SU(0): Data Latency=5 Reg=$r0q
...

Seems like these are (rare) cases then where the defining instruction has an explicit def-op of a subregister, and a RegAlloc-implicit-def of the full register. The using instruction has an explicit use of the *other*
subreg, and an implicit use of the full register. The latency value is set only on the super-register (RegAlloc operand).

• The "implicit-def of the full register" is added when materializing the result of the regalloc in VirtRegMap.cpp, it is not present or necessary during regalloc itself.
• In your debug output I see:

SU(0):   renamable $r1d = LGFRL @seedi, implicit-def $r0q :: (dereferenceable load 4 from @seedi)
...
Successors:
SU(4): Data Latency=5 Reg=$r1d
SU(4): Data Latency=5 Reg=$r0q
SU(3): Data Latency=5 Reg=$r0q
SU(3): Data Latency=1 Reg=$r0d

Given that the actual instruction only writes to r1d I would argue that the latencies on r0q and r0d are "fake". Hence my proposal to ignore the extra operands during schedule dag construction or force their latency to zero. Or do you actually want a latency between SU(0) and SU(3) here?

First: My experience with MachineMemOperands is limited, so feel free to correct me where necessary.

Please read https://llvm.org/docs/MIRLangRef.html#simplifying-mir-files

It would help review if you could describe the exact situation this happens. I am wondering:

Let llvm_test_data() handle directories so we don't need a separate llvm_test_data_dir().

Fix benchmarks I missed before because they aren't built by default.

• properly document new cmake functions and add SUB_DIRECTORY argument to llvm_test_data_dir() for consistency.

Some notes:

• I is only for the benchmarks shipping with the test-suite yet. The things in External will need some additional changes (they will keep working as is, just won't consistenly have their data in the builddir)
• The copying grows the builddir from ~900MB to 1GB for me. If this is a concern then we could add a mode that creates symlinks when the test-suite is running locally; however I wasn't convinced yet it is worth having two modes just for 100MB savings in the builddir.