In D78265#1999445, @vsk wrote:

Sorry about the breakage, I've renamed the variable in 210616bd38d589020b45f8cbbf9f9ef1296f2729.

In D78265#1999223, @chill wrote:

Looks like it's a scope error in gcc <= 5.5.
https://gcc.godbolt.org/z/ECqRJJ

The minimum required version to build LLVM GCC 5.1.
https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

In D78265#1999052, @LukeGeeson wrote:

I cannot build llvm locally using ninja clean && ninja check-all with this patch due to the following error:

AArch64ConditionOptimizer.cpp: In member function 'llvm::MachineInstr* {anonymous}::AArch64ConditionOptimizer::findSuitableCompare(llvm::MachineBasicBlock*) ... no matching function call to 'prev(llvm::MachineInstr&)'

Using the following cmake command in llvm-project/build

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lld" -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_ASSERTIONS=On -DLLVM_TARGETS_TO_BUILD="ARM;X86;AArch64" -DLLVM_ENABLE_SOLVER_Z3=Off -DPARALLEL_LINK_JOBS=2 -DLLVM_USE_LINKER=gold ../llvm

It looks as though harbourmaster is failing as well.
Based on commit hash 3a53806 on master

In D77184#1960473, @thakis wrote:

I pushed Andrew's fix (thanks!) (with minor formatting tweaks) in dbb0d8ecb3a024bd6817ebd8ad8c5c199a51d933 . Let me know if you still see issues.

In D77184#1957632, @thakis wrote:

Thanks!

@aprantl, as per your feedback I have moved the test case to the correct location.

In D57444#1390350, @dstenb wrote:

Sorry for not noticing this before, but we noticed now that the test case is placed under test/CodeGen/X86/, although it is a simplifycfg test. Should it ideally be placed under test/Transforms/SimplifyCFG/?

Thanks @aprantl, @uabelho for your valuable review.

Updated patch to address comments from @aprantl .

Updated patch to remove the negative CHECK. Thanks for your feedback.

Thanks very much for your invaluable feedback and reviews.

Your change seems to cause build failures. The following is the error message:

@vsk: I have updated the patch to remove the test case pr39187-gline-tables-only.ll, as per your suggestion.

Thanks for your comments.

Thanks for your comments. I have updated the patch to address them.

Ping.

@courbet: Your commit seems to cause build issues:

@aprantl, @vsk: Thanks very much for your valuable comments and suggestions.

In D53287#1274290, @vsk wrote:

LGTM, thanks. I'm sorry for the delay in reviewing this.

@vsk: Do you have any additional comments? It is OK for you if I commit the patch?

Address issues raised by @aprantl.

This revision caused builbot failures.

The patch caused some assertions. For more details see PR39243.

@mstorsjo, The change has been reverted. Thanks for creating the bugzilla.

In D52887#1260298, @mstorsjo wrote:

This broke compilation for me, compiling some source files now fail due to triggered assertions, see PR39243 for details. I guess I'll revert this in a while unless there's someone to fix it soon.

Thanks very much to all the reviewers for your valuable comments and suggestions.

Address issues raised by the reviewers.

Address issues raised by the reviewers.

Address issues raised by the reviewers.

Thanks very much to all the reviewers for your valuable comments and suggestions.

Address issues raised by the reviewers.

In D52614#1248058, @dstenb wrote:

It would be good to change the title to reflect what is done in MCP (at least before submitting this).

Thanks very much to all the reviewers for your valuable comments and suggestions.

The general preference seems to land this patch as-is.

@aprantl and @probinson,

Address issues raised by the reviewers.

In D51976#1235508, @aprantl wrote:

Having read the real testcase now, I understand why you are dropping the debug info now. I would like to point out that we could do better. If we extended llvm.dbg.value to take more than one LLVM SSA Value, then we could produce a DIExpression that selects either %add or %sub depending on the value of %cmp. Let me know if you are interested in implementing this, it should be relatively straight forward to do and would come in useful in all sorts of other situations, too.

Address issues raised by the reviewers.

Address issues raised by the reviewers.

In D51976#1232030, @aprantl wrote:

It still isn't clear to me why you need to drop the debug info in the first place (instead of e.g., making it point to the select, or leave it on its operands) but I hope that that will become obvious once I see the actual testcase.

In D51583#1221878, @kristina wrote:

Maybe remove a comma in the comment? A lot of tools don't like trailing newlines.

Up to you, any NFC change is usually fine for this, though you didn't assign anyone as a reviewer.

In D50887#1216171, @vsk wrote:

Thanks, LGTM. If you don't have commit access yet [1], I'd be happy to commit this on your behalf.

[1] https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

@vsk & @aprantl Thanks your reviews.

Update the original patch, as the results from the investigation to replace the algorithm used by 'collectDebugValues', showed some additional complexities.

In D50887#1212594, @vsk wrote:

@CarlosAlbertoEnciso thanks for investigating this and testing out a new way to collect debug values. Given the complexity of replacing the current algorithm, and that it's not strictly related to fixing MachineCSE, I'd recommend keeping the current algorithm (i.e not using MachineRegisterInfo for now). That will allow us to land a fix in tree soon and hopefully iterate on it.

In D50887#1212565, @aprantl wrote:

Do you have OrderMap readily available or would you need to compute it first?

In D50887#1207849, @vsk wrote:

This is looking really good. I think it's great that the test exercises more than just MachineCSE in isolation. Just two more minor comments (inline) --

Following feedback from the reviewers:

Is there anything I can add to this patch in order to submit it?
This patch is blocking the review https://reviews.llvm.org/D44826 which is already approved.

A minor modification to allow the comments for MarkNamespaceAliasReferenced to be picked up by doxygen.

Used clang-format-diff as indicated by @probinson:

@probinson: I tried clang-format-diff and the main format issues are with the test cases.

In D46190#1166775, @probinson wrote:

A bunch of style comments. Maybe try clang-format-diff.

Address review comments from @probinson

@rsmith and @probinson:

In D49426#1165346, @TWeaver wrote:

In D49426#1165146, @dblaikie wrote:

Could you provide a small example dump of the invalid line table you're addressing?

I fed the resulting .elf into dwarfdump with -l and got the following output:

@rsmith Is there anything I can add to this patch?

Update the patch to address the comments from the reviewers.

In D48441#1151889, @ioeric wrote:

Hi Carlos, thanks for letting us know! I sent r336249 to fix the windows
test.

In D46190#1135295, @probinson wrote:

Style comments.
The two new Sema methods (for namespaces and using references) are C++ specific, so SemaDeclCXX.cpp would seem like a more appropriate home for them.

In D46190#1135688, @rsmith wrote:

The right way to handle this is to pass both the ultimately-selected declaration and the declaration found by name lookup into the calls to MarkAnyDeclReferenced and friends. We should mark the selected declaration as Used (when appropriate), and mark the found declaration as Referenced.

We should not be trying to reconstruct which using declarations are in scope after the fact. Only declarations actually found by name lookup and then selected by the context performing the lookup should be marked referenced. (There may be cases where name lookup discards equivalent lookup results that are not redeclarations of one another, though, and to handle such cases properly, you may need to teach name lookup to track a list of found declarations rather than only one.)

It seems that your patch is causing an error in the tests for