DestSourcePair was not is_trivially_copyable with MSVC toolchain so I've updated its members to be pointers instead of references.

Is this good to go now?

@ -> \c

-Rebase
-Move register identity transformation check to DwarfDebug::collectCallSiteParameters()

LGTM! Handling each instruction separately is indeed more safer! Too bad that we can't relay much on MI generic flags such as this one.

Thanks for useful comments @dstenb!

Any additional comments?

-Update addImmediate() commet.
-Add source code producer for test case.

-Include Destination operand in isAddImmediate()
-Exclude description of add immediate instructions where source and destination registers are the same.

-Update lldb test.

Rebase:

-AArch64ISEL part is changed in order do avoid tracking cases when two or more parameters can be in same register.

Adjust comments in MachineFunction.h

-Remove redundant comment.

-Remove unnecessary MIR test elements.

@dstenb you are right! I've tried this example and it is as you expected. We should avoid dereferencing memory locations. I will address those issues in separate patch.

-Address spelling issues.
-Dispatch memory loading instruction descriptions.

Split call site updating functions on move, copy and erase functions.

Thanks for the comment @efriedma! I've looked more carefully this optimization. It turns out that after CopyAndPredicateBlock it is not guaranteed that copied instruction block will be deleted. The deletion of critical call instruction happens after If-conversion during the BranchFolding cleaning when some block does not have predecessors.

Ping.

Should we look for another reviewer or this is good to go?

This looks good! I'm happy to see that implicit operands can be removed from the work-list this efficiently.

LGTM!

Addressing comments.
Thanks for suggestions!

Replacing hard-coded test case values with regex.

From my point of view this is good to go. 'insertAfterBundle' could go as NFC I suppose.

Actually it is not fixed. Bug is related to "-O2" linker optimizations. More precisely to stringpool optimization of merge-able sections. I've located bug in the linker but I didn't have time to get into the details. Here you can find binaries https://sourceware.org/bugzilla/show_bug.cgi?id=16794.

In D64327#1600792, @MaskRay wrote:

Do you mean with the linker option -O2, i.e. the compiler driver option -Wl,-O2, gold can produce incorrect result? If that is the case, I don't think we should revert the patch. ld.bfd and gold default to -O0 (lld defaults to -O1), it is extremely rare for a project to specify -Wl,-O2.

This patch is reverted. Bug in gold compiler is not solved after all. We tested this functionality without optimizations enabled. With the "-O2" flag bug is still present.

The reason why I think it's hard to see that call site information has been removed is that, as far as I can tell there is no way to print the function attributes when doing something like -print-{before,after}-all, so it can be hard to to see where in the pass pipeline the callSites information was dropped. You could of course analyze the MIR and see where the information is likely to have been dropped, but I guess that can be difficult/annoying for larger programs.

Oh, I thought r364506 (D61061) implemented MIR printer support for call site metadata. If not, it's probably worth adding.

This dumps the CallSitesInfo when we producing a ".mir" file. The CallSitesInfo describes an internal state of the MF like the fixedStack, frameInfo etc. I'm not sure how we could make it recognizable in -print-{before,after}-all. Maybe like some metadata extension of call instruction. However, it should be part of another patch.

@dstenb The patch looks good! LGTM! Thanks for working on this!

@MaskRay Thanks for the heads up and review!

Ping. Do we need to explain our concern more precisely?

Thanks for the review and patience!

In D61600#1527581, @aprantl wrote:

In D61600#1527182, @dstenb wrote:

In D61600#1527114, @NikolaPrica wrote:

Have you tested the performance when compiling Clang with ASAN enabled to make sure we're not accidentally regressing because of the more thorough single-location check?

I've built LLVM with '-DLLVM_USE_SANITIZER=Address' using downloaded clang binary to produce private clang binary. I've run tests and compiled latest version of gdb with new private binary and I didn't see any regression there. Is that what you meant?

I think that the idea was to see if this patch increases the compilation time when building such a Clang binary.

(Just make sure to use the RelWithDebInfo build type also.)

Yes. Enabling ASAN on a large C++ code base such as Clang is a great way to see compiler performance problems.
Can you post the output of "ninja clean && time ninja clang" before and after applying your patch to the *host* compiler?

Thanks for clarification!

before

real 46m29,670s
user 325m27,488s
sys 6m17,255s

Have you tested the performance when compiling Clang with ASAN enabled to make sure we're not accidentally regressing because of the more thorough single-location check?

• Rebase.

In D61061#1505375, @qcolombet wrote:

Please provide us with more details about your idea.

When we lower a call we invoke some method to do the lowering to register, frame index etc. The idea would be to extract this code (easier said than done) into its own library and use this API to produce the debug info instead of carrying this information in the MachineFunction.

• Updated comment (single instruction -> single location)

Is this ready to land now?

• Update comments.

• Represent DbgVariable's single value representation by DebugLocEntry::Value insted of representing it with MachineInstr. Such representation will allow us to produce fragment single value representation (for now it is not supported).
• Update DwarfDebug::buildLocationList to check if produced list has single entry that is valid throughout whole variable's scope.

• Address comments for test labels.
• Perform conditional copy for new OutLocs set.