I've only read up to Formal Rules so later sections might change things but I figure it's potentially useful to see a readers thoughts mid-read. I'm pretty sure I've misunderstood the anchor intrinsic based on what I've read of the doc and comments so far.

In D86664#2242821, @aemerson wrote:

In D86664#2240598, @arsenm wrote:

The IRTranslator chooses to omit the G_BR if it can use a fallthrough. Should it not do that either, and the verifier enforce always having G_BR and G_BRCOND?

I think this is a reasonable idea. We discussed this a long time ago internally but ultimately didn't have any strong opinions to change it.

LGTM

Make standalone clang builds work correctly. LLVM_SOURCE_DIR isn't set for those
Small naming change to match that directory

Last one our own testing hit: Fix the case where lack of debug info means we didn't extract dsyms

Fix the non-LTO path. It wasn't externalizing the debug info in single-slice builds

Remove external- prefix

Attempt to fix the windows-specific failure by using posix-style path::append()

In D84565#2173770, @dsanders wrote:

In D84565#2173744, @beanz wrote:

@JDevlieghere you are right, I'm missing the change to how clang determines it needs to pass the linker the object file path. Update coming momentarily.

Isn't that already based on isUsingLTO() and getLTOMode() or is there another bit? I'm looking at darwin::Linker::AddLinkArgs()

In D84565#2173744, @beanz wrote:

@JDevlieghere you are right, I'm missing the change to how clang determines it needs to pass the linker the object file path. Update coming momentarily.

In D84565#2173615, @JDevlieghere wrote:

When you don’t pass any specific options to the linker, it’s going to generate a temporary lto.o file in /tmp and delete it after the link. When dsymutil will try to read that, it won't be there anymore. Unless I'm missing I don't think this is going to work. If it turns out I'm mistaken please add that situation as a test case.

• Comment on the emit+move too

• Add comment referencing the bug

In D84127#2173436, @beanz wrote:

I was talking to @bogner about this. I think this patch is reasonable, but it is important to note that this patch (and the previous version) are both workarounds for a bug in clang. Anytime the clang driver is provided a flag to generate debug info and it is linking temporary object files, clang should insert a dsymutil step. Right now it only inserts the step if there is a compiler or assembler input to the compiler. I filed a bug to track it (https://bugs.llvm.org/show_bug.cgi?id=46841), and I will upload a patch to the clang driver to fix this issue.

In D84127#2161077, @dsanders wrote:

tools/lto/hide-linkonce-odr.ll failed on my test run but it seems to be down to my system rather than the compiler (a library is genuinely missing). Will check if it fails without this change too

tools/lto/hide-linkonce-odr.ll failed on my test run but it seems to be down to my system rather than the compiler (a library is genuinely missing). Will check if it fails without this change too

In D83034#2133989, @arsenm wrote:

In D83034#2133375, @dsanders wrote:

Could you add a test case or point us at a example of an existing test that goes wrong? The description in the commit message is unlikely to be the full story as we already cover loads with non-null MemoryVT's. My best guess is you are attempting to use builtin predicates and custom predicates together. I don't see a reason why that shouldn't be allowed but it's not something that was intended as the goal was aiming to fully remove the custom C++ from the tablegen headers so that tablegen could do some transformations on sextload/zextload and similar to fix DAGISel vs GlobalISel modelling mismatches.

I think the problem is broader than just combining custom predicates and builtin. The emitter here implicitly assumes all of these builtin PatFrag predicates are used exactly as the hierarchy used to define the default/generic load/store patterns.

Yeah, there isn't really a better way to do it as there's no dependency tracking in the importer. GIMatchDAG can track exactly what needs to be available for the predicate to be testable (making it just an issue of how to represent that in the source) but we aren't using that in the importer.

In D83275#2134966, @ychen wrote:

In D83275#2134936, @dsanders wrote:

I was worried for a moment that we'd be losing the ability to print between all machine passes but it looks like -print-after-all covers that now (I don't think that was always the case). So long as we aren't losing any inter-pass dumps this LGTM but I'd suggest giving it a few days to see if anyone was using this in a way that isn't evident from the tests.

-print-after/-stop-after since isel pass does not have commandline name.

IIRC, there's something weird going on in this area. I vaguely remember a problem I never got to the bottom of where there was no name when AMDGPU was omitted but when it was compiled, everybody's pass was called amdgpu-isel. It had something to do with AMDGPU needing additional dependencies and using INITIALIZE_PASS to get them.

I don't know if it is the cause but AMDGPUDAGToDAGISel::ID is actually SelectionDAGISel::ID.

I was worried for a moment that we'd be losing the ability to print between all machine passes but it looks like -print-after-all covers that now (I don't think that was always the case). So long as we aren't losing any inter-pass dumps this LGTM but I'd suggest giving it a few days to see if anyone was using this in a way that isn't evident from the tests.

Could you add a test case or point us at a example of an existing test that goes wrong? The description in the commit message is unlikely to be the full story as we already cover loads with non-null MemoryVT's. My best guess is you are attempting to use builtin predicates and custom predicates together. I don't see a reason why that shouldn't be allowed but it's not something that was intended as the goal was aiming to fully remove the custom C++ from the tablegen headers so that tablegen could do some transformations on sextload/zextload and similar to fix DAGISel vs GlobalISel modelling mismatches.

From that it *sounds* like a DBG_VALUE of an undef vreg doesn't make sense?

In D82765#2121196, @arsenm wrote:

In D82765#2121181, @aprantl wrote:

My MIR is lacking here: What does it mean for a USE to be UNDEF? DBG_VALUE accepts UNDEF as an operand, but it's not clear to me what an undef vreg means / how it can be created.

You can set the undef flag on the MachineOperand.

ProcessImplicitDefs turns

%0 = IMPLICIT_DEF
FOO %0

into

FOO undef %0

You can also hand write this, but I don't think any pass before ProcessImplicitDefs and co should be using these

@vsk + @aprantl: I believe this is going to need an exception for DBG_VALUE. Could you confirm if that's correct?

LGTM

Add only-enable-rule and make it interact with disable-rule correctly.

In D81889#2096115, @paquette wrote:

The point here is to allow people to only enable specific rules, right?

In D81889#2094543, @arsenm wrote:

It's not clear to me what "'!foo-bar' - Enable rules from 'foo' to (and including) 'bar'" means. These have some kind of ordering?

In D81862#2094055, @aditya_nandakumar wrote:

Doesn't https://reviews.llvm.org/D81863 make this unnecessary?

Remove a bit of the next patch that snuck into this one while organizing the patches

Just to give you some context and an overview of the direction, we have some hacks we use to make our LLVM dylib smaller by omitting subcomponents of LLVM that we don't actually use in practice. This is part of a small series of patches to try to eliminate these hacks by rounding out llvmbuild's support for optional libraries. This patch allows libraries to be included by default and optionally disabled, and it will be followed by a small bugfix for --enable-optional-components, and then the addition of an optional_libraries to go with the existing required_libraries.

LGTM

LGTM

I'm not sure this is the correct instance to remove; now the visible
ordering is different. Now you will typically see the one erased
instruction message before all the new instructions in order. I think
this is the more logical view of typical legalization changes,
although it's mechanically backwards from the normal
insert-new-erase-old pattern.

LGTM

LGTM

LGTM once all other reviewers are happy with this patch.

You don't really need to wait for all of us but you should probably wait for @MaskRay and @atanasyan as they had comments.

In D77446#2043308, @djtodoro wrote:

In D77446#2031004, @djtodoro wrote:

Thanks for this work! I've missed this, since it was not marked as 'debug-info' project.

I think we should document this at https://llvm.org/docs/SourceLevelDebugging.html#testing-debug-info-preservation-in-optimizations.

The D80052 will address this comment.

Thanks for working on this. I had a couple inline comments.

In D78796#2025835, @nlguillemot wrote:

In D78796#2025650, @dsanders wrote:

I'm inclined to agree that the patch series as-is doesn't really warrant the iterators as the interface as no callers have been updated. However, I also don't see much that's iterator specific (ULEBifier would be roughly similar code leaving the iterator portion as trivial wrappers on the ULEBifier) and there are a few places (particularly in tablegen) that are emitting LEB's into containers where the loop to add bytes one by one is just noise and something like std::copy(to_uleb(...), std::back_inserter(Table)); would be somewhat nice. The loop could easily be hidden in something like append_uleb(Table, ...) though I don't think there's a strong argument for (or against) iterators.

What I would suggest is separating out the byte-sequence generation into a ULEBifier as David suggested but still keeping the iterator object as a thin adapter (effectively implementing that while loop) to support inter-operation with STL functions like std::copy.

What is the core issue with the iterator interface that makes it desirable to have something like ULEBifier<T> instead?

I'm inclined to agree that the patch series as-is doesn't really warrant the iterators as the interface as no callers have been updated. However, I also don't see much that's iterator specific (ULEBifier would be roughly similar code leaving the iterator portion as trivial wrappers on the ULEBifier) and there are a few places (particularly in tablegen) that are emitting LEB's into containers where the loop to add bytes one by one is just noise and something like std::copy(to_uleb(...), std::back_inserter(Table)); would be somewhat nice. The loop could easily be hidden in something like append_uleb(Table, ...) though I don't think there's a strong argument for (or against) iterators.

LGTM

In D78245#2010879, @thakis wrote:

@thakis Could you link to previous discussions about %T? I'm sure there are reasons for the way things are today, but I'd like to understand them.

I remember reading the discussion, but I failed to find it just now. @dsanders added the docs that say "deprecated"; maybe he remembers where the thread on moving off %T was?

LGTM but double check -verify-machineinstrs works with -run-pass

I don't think it's safe to remove Support since llvm/lib/TableGen/CMakeLists.txt doesn't mention it but adding TableGen to the existing list would LGTM

LGTM