I note from an earlier comment that updating VS2022 with latest patches solved this issue for @srj
But is the issue similar for the minimum required version of MSVC (currently Visual Studio 2019 / v16.0 )??

It passed across all windows buildbots, which includes VS 2019. Example: https://lab.llvm.org/buildbot/#/builders/13/builds/33254

In D144503#4204120, @ronlieb wrote:

also seeing similar error in downstream windows build of llvm.

In D144503#4201034, @srj wrote:

There is also a similar breakage in InstrInfoEmitter.cpp:

C:\build_bot\worker\llvm-17-x86-32-windows\llvm-project\llvm\utils\TableGen\InstrInfoEmitter.cpp(120): error C2440: 'initializing': cannot convert from 'size_t' to '_This'
        with
        [
            _This=llvm::Record *const &
        ]

Fix up ASSERT_TRUE to pass messages correctly

Thanks for the speedy review Jannik
Changes made

Got it. But it is still not good to modify the upstream due the requirement from the downstream. (I know this happens in llvm occasionally. But it is still not good, right?)

I think it may better for you to contribute the other materialization options you mentioned. Or you can try to add a unittest if you don't want to contribute the downstream extension to public for any reason . Otherwise, even all people here get in consensus. It is possible that other developers found the coding here is redundant and optimize it with a NFC patch directly. (This happens a lot in llvm)

Add unit test demonstrating use of extra rematerialization callback

In D142621#4115184, @dstuttard wrote:

In D142621#4114357, @ChuanqiXu wrote:

While this is innocent and no harmful, I prefer to land this after we found a usage for this someday. It wouldn't be a problem to do such a NFC change any times.

We have a use-case for this change for one of our backends.

Address review commit (sorry Jannik - missed it before)

In D142621#4114357, @ChuanqiXu wrote:

While this is innocent and no harmful, I prefer to land this after we found a usage for this someday. It wouldn't be a problem to do such a NFC change any times.

Rebase on fixed version

clang-format change

Removed assert that was incorrect (and causing build-bot pre-checkin failures)

Rebased

More changes based on feedback

Thanks for the feedback - see the comments and the udpated patch(es)

Addressing reviewer comments

Addressing reviewer comment

LGTM

Well, this LGTM, but probably need some feedback from others.

Managed to get arc diff to work!

Updated - adding test for builtin

Address review comments

Updating based on review comments

This fixes the issue I was observing. Thanks.

Thanks (sorry I missed it on the original comments)

Is something required for system-libs.windows.test too?

I think that the reason we don't see the same failure for e.g. OpenACC is that there's a unittest/Frontend that requires OpenACC - if I comment out that subdir in the unittests/CMakeLists.txt it fails llvm-config test in the same way for OpenACC. Similarly, if I add FrontendHLSL as an LLVM_LINK_COMPONENT in unittests/Frontend/CMakeLists.txt (even though it isn't), the static lib is generated and llvm-lit llvm-config test passes.

Include address space AS in the pointerto call

Updated for review requests

Thanks - removed the test and made the changes.

LGTM

Added a test

Updated with review suggestions

I wasn't sure whether Mesa would also have issues - I was going to follow that up, so thanks for the info.
I'll update as you suggest to !IsShader

Additionally, not everything marked as inreg actually contributes to the NumUserSGPRs - so the knowledge about which arguments are and aren't would need to be added to the backend.

In D129566#3645692, @arsenm wrote:

I don't see why PAL would get special treatment here. I think the problem is we don't really try to account for the formal arguments in the user SGPR count. A better fix would be to track that accurately

Adding check lines back into mad_u64_u32.ll

Updated tests to include 1030 specific run lines

Add test updates

Forgot to update the tests. Stand by.

Well spotted. The problem is that it's difficult or impossible to persuade globalisel to use the svs addressing mode in the first place, because it assumes that frame offsets are divergent so doesn't put them in an sgpr for us. See AMDGPURegisterBankInfo.cpp:

I note that this includes global isel changes - but none of the tests are testing it?

This looks reasonable to me, but I think we need someone else to confirm too.

LGTM

Pushed an earlier commit by mistake

Picked this issue up in some testing - seems a logical fix.
I have a reproducer, but even the cut-down isn't that clean - do you think this requires a test?

Spelling mistake (and rebase for test changes)

Updating test in light of review suggestions

Updating with link to change for which this is a pre-commit

Pre-commit test