LGTM

LGTM. A very nice and useful improvement.

I think getting rid of this option is good for us and our users in the long term.

LLVM’s C++ interfaces do not provide a stable ABI across all configuration flags (which is not uncommon for C++). As a result llvm-config and the LLVM CMake exports provide the options LLVM was built with for the user.

I don't think this PR is correct. It seems like we should be able to ensure that the analysis result remains preserved after the metadata is generated allowing us to re-use the analysis result that was used to generate the metadata.

In D142279#4106708, @foad wrote:

@beanz FYI - I think I heard you were a cmake expert!

LGTM

LGTM

LGTM. This looks pretty straightforward and similar to other changes you've been making.

LGTM

In D142405#4081473, @aaronpuchert wrote:

This doesn't affect us internally because we have cmake_minimum_required(VERSION 3.13.4), but should we add cmake_policy(SET CMP0057 NEW) to the exported files? Perhaps in a PUSH/POP?

LGTM. Thank you for the nice cleanup :D

In D96670#4074988, @aaronpuchert wrote:

Yeah, I don't usually do this. But this has been sitting for a very long time with a couple of pings and no attention.

In D142404#4074985, @mstorsjo wrote:

No, here I disagree. LLVM_HOST_TRIPLE is documented as Host on which LLVM binaries will run, not as the host where I'm currently compiling it. We can easily infer the details of the OS where we're doing the build, but usually much less so for the cross target, where the cross compiled LLVM will run.

In D142404#4074962, @mstorsjo wrote:

Plus, since LLVM_TARGET_ARCH is the target to use for JIT generation, it essentially needs to be the same architecture as the host on which LLVM is going to run, so it can't really be set to a wildly different arch anyway?

Neither the current doc, nor the proposed change are really always right, and they are doing different things.

LGTM. Thank you for taking the time to update the docs!

LGTM.

Yea, this makes sense for Windows. LGTM.

“I can’t find someone to review this so I’m just going to merge it” is not okay.

A little bit of code cleanup. While working on the next patch in the series I refactored some of this code and thought I should just bring the refactoring back.

@python3kgae, this change introduced a bunch of warning spew because it is using an API that was deprecated shortly before the change merged. Can you please address this?

LGTM!

Adding Unused to VertexPSVInfo

Re-using the VerifyDebug action really doesn't make sense. That's not what the DXIL validator does, and it will be a source of confusion forever.

The convention that find_program uses is to cache the variables, which causes them to be defined at global scope. That also avoids needing to recompute filesystem lookups in incremental builds, which is desirable.

In D141649#4050286, @python3kgae wrote:

So no resource support in this PR?

All the other changes going into the DXContainer object and yaml support are aiming to be more full featured, and the object and yaml tooling changes are separate from the implementation change.

LGTM

LGTM

LGTM

A few questions. Does this pass work with handles passed as parameters to functions? What about different functions that refer to the same global? Does that result in multiple createHandle calls?

It is probably worth adding some unit tests to test the CBufferDataLayout class.

LGTM

You have some stub code in here to handle emitting shader flags, but the value is always set to 0 resulting in nothing being written and thus no tests.

LGTM

Updating based on PR feedback from bob80905. Thank you!

In D133379#3984033, @jsilvanus wrote:

Sure -- but still the behavior on such cases should be clear, even if it is not relevant for DXIL. I just stumbled over the formulation

LGTM. @bob80905 if you haven't already, you should follow the steps here (https://www.llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access) to get commit access.

Need to byte-swap the hash structure on big endian hosts so that it is encoded as little-endian.

In D133379#3962893, @jsilvanus wrote:

How should this behave on overaligned element types? For example, with a i16:32 data layout (as in the old DXIL data layout),
the second element in <3 x i16> will not be 32-bit aligned, even if the vector is.

I think I would broadly classify that there are two problems you're fixing here:
(1) Under-specified dependencies.
(2) A cyclic dependency on PointerTypeAnalysis

LGTM

In D132413#3886280, @fcloutier wrote:

Failing tests are from clangd and WebAssembly backend, which are not touched by this change.

Please add a check line to the embed-dxil.ll test to verify that the triple is restored after embedding the DXIL global, otherwise LGTM.