In D86644#2514125, @GMNGeoffrey wrote:

In D86644#2513914, @wolfgangp wrote:

In D86644#2513898, @GMNGeoffrey wrote:

FYI I'm seeing clang diagnostic errors, I think from this commit:

Should be fixed now.

I'm still seeing some. Looks like your fix didn't get all of them:

In rGd38be2ba0e4ebfed4c13ab79f3a8631011d185eb#971631, @leonardchan wrote:

Hi. It seems this commit is causing this failure on our aarch64 bulder (https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-arm64/b8857452110716658368):

In D86644#2513898, @GMNGeoffrey wrote:

FYI I'm seeing clang diagnostic errors, I think from this commit:

Sorry about the missing files. Some hiccup while generating the diff.

Based on Andrea's request to use an OutputKind rather than overriding the printView method in a separate class for JSON output

This change is trying to use a more streamlined interface based on Andrea's feedback. I removed the printJSON method from the PipelinePrinter and the singleton object that was used to list the instructions.
Instead, each implemented view now has a <ViewName>JSON derived class that overrides the printView method. When JSON output is requested, objects of these class types are added as views. This leaves the PipelinePrinter largely unchanged.

Apologies for the long delay, I'm finally ready to resume work on this feature. The update does the following:

Isn't COMPILER_RT_HAS_FLOAT16 generated using the host compiler? If so, using the same flag for a compiler_rt test could fail if the host compiler supports f16 and the target compiler does not.

This seems to fail on some buildbots: e.g. here

The test seems to fail on some Linux buildbots, see here. Please review.

In D86644#2333464, @andreadb wrote:

Hey Wolfgang,
are you still looking at this patch? Do you have any updates?

This commit seems to cause a test failure on some bots, e.g. here.

In D86644#2239673, @lebedev.ri wrote:

Can it simply be autogenerated, without omitting details?

After some consultation with @andreadb this is some more extensive refactoring:
We create a new base class InstructionView that is meant for views that deal with individual instructions (e.g. print them). The 3 members MCIP (the instruction printer), STI (subtarget info) and Source (the array of machine instructions) live there and can be accessed via getters (they could be made protected as well). Printing the instructions is effected by using a string member in the new base class (which must be mutable along with its associated stream).

Here is another proposal: We're using statics for the Instruction string and the stream to write it and return a reference to it.
The users all immediately write the string to a different stream. This should have even fewer allocations than the original code.

Addressed review comment:

• using enumerate() instead of an explicit index variable

In D86177#2225748, @andreadb wrote:

Nice!

Thank you Andrea!

I agree with Roman in that we should not guarantee stability of data and/or structure.
Also, changes to the output structure should always be advertised (for example, by adding a line in the release notes), so that people are always aware of it.

Addressed review comments:

• Using MutableArrayRef
• Using range-based for loops with zip
• Using default initializers with InstructionInfoViewData

In D68620#1995191, @dblaikie wrote:

In D68620#1729184, @rupprecht wrote:

It looks like this caused a very large increase in binary size (627M->686M). Is that expected/has anyone else observed this?

sorry I didn't see this (somehow ended up muting this thread) - which binary built with what flags? could you run bloaty or otherwise compare the objects before/after. I'd expect some growth in linked executable size of a non-split, optimized debug build, but that seems a bit more than I'd expect/seems to be observed elsewhere so far as I know.

Shortly after this patch went in, we started to see intermittent failures with the unittest TUSchedulerTests.ManyUpdates. The build server (Windows) we're running it on is quite powerful and our own hand-run tests only take about 400 ms, so we don't really understand what could be happening. Any help would be appreciated.

LGTM, with a slight caveat regarding reporting an error for degenerate stubs of string offset sections.

I'm unfortunately still getting a loop in DAGcombiner with this one (llc -mattr=+avx on linux). If you make the 1.000 in the select instr into 0.000 it finishes.

define float @test(<8 x float> *%a0) {
entry:
  %0 = load <8 x float>, <8 x float>* %a0, align 32
  %vecext = extractelement <8 x float> %0, i32 1
  %cmp = fcmp oeq float %vecext, 0.000000e+00
  %cond = select i1 %cmp, float 1.000000e+00, float %vecext
  ret float %cond
}

I applied your patch to ToT, but now I'm seeing a loop with the following IR (on linux). Seems still stuck in DAGCombine.

In D67998#1683795, @jhenderson wrote:

In D67998#1682945, @wolfgangp wrote:

Yes, good observation. There is also the question what to do if the sizes of 2 different entries with the same address don't match. This should probably elicit a warning.

<snip>

1. Display the first entry size, for each symbol. This is probably the simplest.

For some reason, I didn't see your response until I had posted my updated. I basically implemented number 3.

1. Rebased with the latest upstream changes.
2. Addressed review comments as follows:
   • Keeping track of stack sizes already dumped so that duplicate entries only are printed once.
   • Issue a warning when a duplicate entry with a different stack size is encountered.
   • Simplified code by collapsing some warning code into a lambda.
   • added the appropriate test cases for the changed functionality and added one for STT_NOTYPE.
   • added an early return in a case where no valid symbol is found.

In D67998#1682076, @jhenderson wrote:

One issue that just occurred to me, is what happens if you still have the stack size entry in this case? Something like:

<snip>

I think this is undesirable. Perhaps you need to track whether a stack size entry for a given address has been printed yet, and skip it if it has?

Addressed review comments:

This sentence in the description:

The reasons for this are unclear, so this patch adds the option to llvm-strip.

We do not add options merely for "unclear" reasons. It is because the option is a good fit for llvm-strip.

I'm a bit unclear on what this means:

Addressed review comments:

In D65313#1611137, @rupprecht wrote:

In D65313#1609287, @wolfgangp wrote:

Left the question about whether --stack-sizes should be include in --all open. This is still a discussion point.

Do you have a discussion going on somewhere you can link to?

Addressed review comments:

• renamed one of the printStackSize() functions.
• removed superfluous check for nonrelocatable object
• Used SectionRef::getName() instead of the explicit sequence to extract a section name.
• Fixed the calls to the DataExtractor() constructor to use Obj->isLittleEndian()
• Added some comments to explain the error handling policy.
• Moved a SectionName variable definition closer to the use point.

• Undertook some refactoring of the change with the following motivation: The printStackSize* methods do mostly extracting and validating, printing the output is only a minor part. Yet they were methods of the GNUStyle class, which would have made it more difficult to implement LLVM style dumping. So I moved them mostly into the base class (DumpStyle), and delegated the printing mostly to a new method printStackSizeEntry(), which is style specific.
• Addressed the formatting comments
• Added 2 test cases (demangling and verifiying the 'not implemented' message we get from llvm-readobj.
• Explained the rationale using consumeError().

Addressed review comments as suggested.

LGTM

@danielcdh @wolfgangp -- Would switching to line 0 locations for hoisted instructions have an adverse effect on Sample PGO?

So this basically prevents the debugger from displaying an inconsistent program state when some of the sideeffects from a line preceding its current stopping point have not been completed yet. Makes sense to me. One minor concern: If the sunk instruction does not get sunk past the next source line, we may create some unneeded location list entries, e.g.:

• Addressed review comments on code structure and clarifying comments.

Changed the function names from isSpillInstruction/isRestoreInstruction to getSpillSize/getRestoreSize.

• Added methods to MachineInstr to recognize Spills and restores and have AsmPrinter and LIvedebugValues use them
• Unified tranferSpill and transferRestore functions.
• Addressed other minor review comments.

LGTM