In D75918#1978445, @jsji wrote:

I will commit for you.

ping

In D75918#1960302, @bcahoon wrote:

I've tested this patch with our version of LLVM and it passes our tests. I'm ok with adding the patch as-is, as long as no one else would object.

In D75918#1953795, @bcahoon wrote:

Unfortunately, I've had no luck producing a test case that generates this type of graph.

ping

In D75424#1939185, @jsji wrote:

I will commit it for you.

I can't seem to get commit access to GitHub (already emailed llvm-admin twice)
Would anyone be kind enough to commit the change on behalf of me? Thanks!

In D75918#1917282, @bcahoon wrote:

Yep, stores to physical registers will create output dependences, which can be done using inline assembly. At least, most of the code in the pipeliner that deals with output dependences is exercised using inline asm code.

The only Hexagon instructions with two outputs that the compiler generates are the post-increment load instructions, and it would be difficult to create a chain of these. There are some esoteric instructions, but they are typically generated through intrinsics only. I can try to create a test for this...

In D75918#1916156, @bcahoon wrote:

Hi @lsaba - this fix looks good to me. Thanks! It will be challenging to create a test case on Hexagon due to the output dependences. How to do get so many output dependences on your target?

re
Thanks for the fast reply!
we have an implicit physical register that certain instructions define, thus having several instructions in the code would create these output chains, the most similar to this that I found in Hexagon was the usr_ovf register, but I also saw that the Output Deps for these registers get removed in HexagonSubtarget::UsrOverflowMutation::apply.

In D56084#1356329, @aprantl wrote:

I *think* I fixed it for good in in r351077.

In D56084#1351317, @aprantl wrote:

You have to use clang in order to compile with clang module support enabled. If your host compiler is GCC, you will have to build clang first and then use that clang to compile clang again using all the MODULE cmake options.

In D56084#1351317, @aprantl wrote:

You have to use clang in order to compile with clang module support enabled. If your host compiler is GCC, you will have to build clang first and then use that clang to compile clang again using all the MODULE cmake options.

In D56084#1350146, @aprantl wrote:

Hmmm.. my fix didn't go far enough. I reverted this patch once again, so we can sort this out without pressure. As long as you make sure that compiling with -DLLVM_ENABLE_MODULES=1 works, feel free to recommit at any

In D56084#1350118, @aprantl wrote:

I committed a rather horrible workaround in r350650, but it would be great to revert that again.

What you need to test particularly is building with -DLLVM_ENABLE_MODULES=1.

In D56084#1348719, @aprantl wrote:

Looks like these are unrelated failures. TestQueues started acting up last week because of a concurrency issue. I would recommend you attempt to re-land your commit again and see if any new failures show up. If you are unsure, feel free to ask here or on #llvm on IRC.

In D56084#1348493, @aprantl wrote:

In D56084#1347564, @lsaba wrote:

In D56084#1346142, @sgraenitz wrote:

@aprantl Found a workaround for the lldb-cmake bot: https://reviews.llvm.org/rL350346

I tested it locally with your patch reapplied and the error is fixed. Sorry again for the hassle. Please submit your patch. Thanks

@aprantl , @sgraenitz , I tried to resbumit the patch in https://reviews.llvm.org/rL350493 but unfortunately issues with the modules still appear: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/314/steps/test/logs/stdio

it is now able to build but the lit test SymbolFile/PDB/class-layout.test is failing:

$ ":" "RUN: at line 4"
$ "E:\build_slave\lldb-x64-windows-ninja\build\bin\lldb-test.EXE" "symbols" "E:\build_slave\lldb-x64-windows-ninja\build\tools\lldb\lit\SymbolFile\PDB\Output/ClassLayoutTest.cpp.exe"

command stderr:

error: Module has no symbol vendor.

error: command failed with exit status: 1
$ "E:\build_slave\lldb-x64-windows-ninja\build\bin\FileCheck.EXE" "E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\lit\SymbolFile\PDB\class-layout.test"

command stderr:

E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\lit\SymbolFile\PDB\class-layout.test:15:8: error: CHECK: expected string not found in input
CHECK: Module [[MOD:.*]]

^

<stdin>:1:1: note: scanning from here
Module: E:\build_slave\lldb-x64-windows-ninja\build\tools\lldb\lit\SymbolFile\PDB\Output/ClassLayoutTest.cpp.exe

I have reverted the patch again for now as I am unable to reproduce the issue locally, your help will be much apprecicated

Even though this error also has Module in the name, this is a completely unrelated issue. Unfortunately Module is a terribly overloaded term. Are you sure that this was actually caused by your commit? It sounds unlikely.

In D56084#1346142, @sgraenitz wrote:

@aprantl Found a workaround for the lldb-cmake bot: https://reviews.llvm.org/rL350346

I tested it locally with your patch reapplied and the error is fixed. Sorry again for the hassle. Please submit your patch. Thanks

In D56084#1346142, @sgraenitz wrote:

@aprantl Found a workaround for the lldb-cmake bot: https://reviews.llvm.org/rL350346

I tested it locally with your patch reapplied and the error is fixed. Sorry again for the hassle. Please submit your patch. Thanks

In rL350290#304459, @fhahn wrote:

Looks like the patch has just been reverted in rL350345. Please let me know if you have problems reproducing the failure.

Ping2
@craig.topper is this okay with you?

Ping

In D55642#1331489, @nikic wrote:

LGTM, with a small nit.

In general, I'm not sure whether the overall behavior here makes sense. Especially in the case where the blocking stores don't come from different BBs, it would make more sense to me to preserve the last (in program order) of the overlapping stores, rather than the smallest, as that's the store that will be forwarded. If the stores come from multiple predecessors (like here) it's less clear which choice is right.

But in any case, that would require larger changes and shouldn't block this assertion fix.

no comments from my side (LGTM)

done @craig.topper

fix @craig.topper comment

ping

In D41330#1069883, @thegameg wrote:

This seems to break the Machine Verifier. I filed PR37153. Do you mind taking a look please? Thanks!

In D41330#1069883, @thegameg wrote:

This seems to break the Machine Verifier. I filed PR37153. Do you mind taking a look please? Thanks!

@zvi the changes in this commit were added to https://reviews.llvm.org/D41330 and committed already. I will close this patch

Updated with @craig.topper comments

In D41330#1049332, @niravd wrote:

The changes from just D43619 look correct but I haven't looked deeply at this part of the patch. From the comments it looks like the patch is a bit out of date as std::map should have been converted back to switch statements.

ping @ reviewers, if there are no more comments i'd like to re-submit this

Fixed comments in review https://reviews.llvm.org/D41330

fix comments from @niravd and @craig.topper in review https://reviews.llvm.org/D43619#inline-381377 (closing that one)

Fix the reported bug (added case to bug 36346, based on code provided by Richard Smith )
and limit the transformation to 64bit only

@chandlerc there's a good chance this solves the miscompilations you are seeing

@chandlerc Thanks for your comments.
still checking the option of using tablegen for some of the tables.
Any luck with the reproducer?

committed in https://reviews.llvm.org/rL324835

fixed TRI @craig.topper

addressed @craig.topper 's comments

ping to reviewers

addressed @craig.topper comments

addressed comments from @RKSimon and @craig.topper

Pinging again,,

Ping.

addressed @hfinkel comments.
ping to reviewers.

add MMO info

Addressed Craig's comments

@RKSimon No more comments from my side

LGTM after fixing the minor comments

3-Ops LEA are costly starting target SandyBridge , is there a limitation in the code for the targets this transformation works on? If not I think there should be.
you can check the Slow3OpsLEA feature for the full list of targets.

In D35014#859838, @jbhateja wrote:

@lamas, @reviewers, comments have been taken care. Let me know if anything else.

In D35014#859838, @jbhateja wrote:

@lamas, @reviewers, comments have been taken care. Let me know if anything else.