In D46138#1079876, @aprantl wrote:

I'm sorry if this was discussed earlier... Why are the template parameters attached to the variable, rather than the type?

Hi there :)

In D46119#1079870, @craig.topper wrote:

The only place you could put it is in include/llvm/Support. I suspect this is why the content was duplicated like this.

Sorry, I'm a bit confused.

You can use attributes to force inlining or non-inlining to create a
particular scenario.

Is there anything else in the "-w" namespace other than the literal "-w" so
far?

FWIW I don't fundamentalyl object to also having something like -wtest.
Probably needs a better name though (unfortunately the double-negative gets
confusing... - like you want to describe the set of diagnostics that should
not be used in test code, so that as a group might be "-Wnon-test" but then
"-Wno-non-test" is pretty awkward) - probably worth chatting to Richard
Smith about that, I reckon.

Not really my area of expertise - just skimming the commit history for the file it looks like Quentin did a bunch of nearby work, so perhaps he can help?

Sounds like sort of "-gno-really-line-tables-only" (I kid, somewhat)

I'd say if we can't actually get it totally clean & enable the warning, maybe it's not worth doing all this? & you (& others in your situation) can contribute patches on an as-needed basis?

I've pushed this further - it builds now & has no fallback in libBasic to the old behavior/table.

+1 - I'd rather not have a special case for filter+reverse if we can help
it. If filter can be made bidi so it "just works" without reverse knowing
anything about it, that seems better.

ANyway, seems OK.

OK - so would "skipDebugInstructionsForward" ever need the end iterator?
since it should always come across a non-debug instruction (all valid BBs
have at least one non-debug instruction, the terminator - though I guess
you might call this function on an invalid basic block during
construction/intermediate state changes, etc).

In D45711#1069489, @aheejin wrote:

No, it was producing the correct answer. As long as Header was not empty, whether that part of the code was Header or MBB didn't affect the result of the code.

In D45711#1069483, @aheejin wrote:

I don't understand. The other change (D45648) was a bugfix, and MBB and Header are not the same thing, which was the reason I fixed that bug. A bugfix is not supposed to be NFC, right?

So does this make the other change NFC (under this assertion)? (ie: if the assertion is true, is "MBB" and "*Header" equivalent/the same thing?)

I'm not sure this is a practical direction to pursue - though perhaps
others disagree.

In D45657#1068834, @vsk wrote:

I like the direction this is going in :).

I think having an API like 'instructions_no_debug' or similar in BasicBlock would make the interface friendlier. For one, it'd signal to readers that skipping debug intrinsics is an established pattern. Also, I think it would be easier to change if, e.g we wanted update all existing users of the API to skip lifetime intrinsics too.

Fix up a few comments & then it seems fine.

In D45628#1067375, @aprantl wrote:

Which compilers / platforms generate / support this? Is this an ELF-only feature?

But I don't think we start emitting the CU as we are going, right? I
thought it was handled much like the TUs would be by this patch - built...
And then emitted once it was fully built. (Indeed because new attributes
can be added to DIEs, forward references to future DIEs can exist, etc - we
currently have no support for streaming dwarf emission - the entire DIE
tree is built in memory, then emitted)

This would regress memory usage significantly - see r260578 / D17118 for more details. (17% decrease in memory usage when this was implemented)

Great - thanks for sticking with it, sorry for my dodgy explanations!

I meant that lib/LTO/LLVMBuild.txt should include "ExecutionEngine" in its
required_libraries. I wouldn't expect any changes would be needed to
tools/lto/* or tools/llvm-lto/*.

That sort of surprises me - if that were the case, there would be no point
having LLVMBuild.txt files in any libraries, I would think?

I meant what happens if you remove the changes to tools/lto/LLVMBuild.txt
and tools/llvm-lto/LLVMBuild.txt and /only/ have a change to
lib/LTO/LLVMBuild.txt - is that sufficient to make the build work?

Finish functional prototype - this now does remove the dependency from Basic to the various other libraries.

Is it sufficient to put the dependency MCJIT in lib/LTO/LLVMBuild.txt (since that's where the dependency on is) & not in the tools that /use/ the LTO lib (I expect they should pickup the indirect dependency implicitly?)?

I realize it's non-trivial to keep these all in the same list - but I'd
rather not create a new list only to remove it again later if it can be
helped. What particular issues did you encounter when attempting to use a
single list?

0.6% increase to debug_line (assuming you checked & this didn't increase
anything else? Can't see how it could, but just checking - /maybe/ some
inlining/debug_ranges stuff?) seems acceptable to me.

Hah, wow - didn't know you could include LLVM headers in godbolt code...
that's super handy :D

Seems fine to me

You'd have to add the ExecutionEngine library as a dependency from the LTO
library - by modifying lib/LTO/LLVMBuild.txt - then it should link, I would
think.

Do you have the stats on this change in terms of line table size increase?

FWIW - I had some thoughts on this a while back:
https://reviews.llvm.org/D4313

Looks like ObjectMemoryBuffer is part of ExecutionEngine, so having libLTO
dependent on ExecutionEngine seems basically correct for the dependencies
that are there already. Whether or not those dependencies should be there
is another question, I guess. (maybe ObjectMemoryBuffer should be moved
somewhere more common, etc - but I wouldn't worry about doing that unless
the LTO dependency breaks things)

Out of curiosity - what was the motivation for this change? I'd expect that
copy would be faster than repeated push_back (though maybe LLVM can
optimize it to match)

Switch this to a C++ style static_cast and go ahead. Thanks!

Test case?

Probably best not to create another list on the DISubprogram - you might be
able to generalize the existing list to contain "anything that might be
optimized out"

Rather than adding another list (if/when it comes to that) - we might want
to generalize the existing list to contain any/all local entities.

This still doesn't /sound/ right to me - the triviality of a class doesn't
seem like it would correctly allow you to deduce the triviality of specific
special members? Because the the class would only be considered trivial if
all the special members are trivial, probably?

I'm still not sure this description/name matches the examples given in the
other review from Reid.

Makes sense to me - I haven't applied/tested the patch against some recent
commits to LLVM I've made that broke gold/binutils ld based builders but
didn't break for me locally (using lld), but assuming it catches those -
yeah, it'd be useful to me & I'd totally turn it on locally & suggest
anyone else using lld with LLVM to do so as well.

iterator or iterator-like thing. One option would be a callback with Error
when the iterator is retrieved - and that's called back for any error (&
the user wouldn't need to differentiate the different kinds of error - the
iterator would stop when it couldn't continue). Or an Iterator-like but
not-actually-iterator API that exposes the Error during iteration (but
that'd probably look a bit awkward & be something like pair<Error,
Optional<T>> or, yes, as suggested - a separate Error type to communicate
"fail and cotninue" separately from "fail and stop"... )

I was picturing the parser being the thing that exposes the iterator - so
it would be an internal detail & wouldn't really warrant an extra Error
type - but I haven't thought about it too hard & maybe that doesn't make
sense?

I'm still a bit undecided about new Error types here - I think exposing an
iterator (or iterator-like thing) rather than special changes to the offset
or an error kind the user checks to see whether to iterate might be better?

That's fair (re: cast over &*)

Does the proposed clang change (using 'isTrivial') match MSVC's behavior in
these cases?

Yeah, +1 for verifying whether or not DIFlagTypePassByValue/Reference is
sufficient for this.

Comment might need to be reworded, but might not even need a comment - if
we build with the warning on by default anyone who attempts to remove it
will find out...

I'd probably guess that the best way to expose this is to use a higher
level construct than a raw offset exposed to the user of the API - some
kind of iterator should be exposed & then if parsing can't continue, it
doesn't continue. Rather than exposing the raw offset to the user & having
them have to handle passing it back in, or detecting when not to do so.

Fair - the minor 'duplication' seems fine, given it skips the work of mapping the llvm::Type to an EVT, etc...

Yeah, before committing to any new asm syntax, worth a conversation on
llvm-dev with the usual crew & hopefully we can rope in some folks from GCC
or whatnot. It might even be worth discussing on dwarf-discuss, even though
it's not technically a dwarf issue. (& if you're feeling up for it - if
we're having conversations about assembly syntax extensions anyway, there's
a couple of others that might be nice - the ability to specify a CU for
line table entries (LLVM can produce multiple line tables with its
integrated assembler, for use with LTO and ThinLTO, but falls back to
producing a single line table when producing textual assembly) - and maybe
one day the address pool stuff we've talked about)

Seems good to me

You mention a missed case in the description - where the target of a using decl is used and that causes the unused-using not to warn about the using decl? That seems like a big limitation. Does that mean the 'used' bit is being tracked in the wrong place, on the target of the using decl instead of on the using decl itself?

Historically Clang's policy on warnings was, I think, much more
conservative than it seems to be today. There was a strong desire not to
implement off-by-default warnings, and to have warnings with an
exceptionally low false-positive rate - maybe the user-defined operator
detection was either assumed to, or demonstrated to, have a sufficiently
high false positive rate to not meet that high bar.