Hi Quentin,

I've addressed the comments in this review and done the commit based on the LGTM in https://reviews.llvm.org/D40333, which is the "end version" of all these refactoring changes after more refactoring and clang-format.

Matthias, did you get a chance to look at these changes (and at the changes in https://reviews.llvm.org/D40330) ? Are you OK with them?

ping

Ping

Ping

Changes based on comments in https://reviews.llvm.org/D40333

I've uploaded a new review here and in https://reviews.llvm.org/D40330.

Added .addreg(Reg, RegState::ImplicitDefine)

Addressed comments.

In D15075#940711, @dolson wrote:

Hello,

In the process of upgrading from clang 3.6.1 to a newer version, I ran into this new error and thus imported the new intrinsics from intrin.h for rep movsb and friends. I see several discussions in this thread about how having the registers solely in the inputs list is not sufficient for something like "rep movsb" because the modified registers will not be clobbered, however none of these suggested changes made it into the eventual intrin.h.

I found that using the versions of __movsb and __stosb that are at the head revision intrin.h produced bad code generation vs the versions with the clobbers. Note this is on PS4 under the older clang 3.6.1, but I don't see anything in this CL that would update the clobber behavior for newer versions of clang.

Shouldn't the intrinsics be updated to use input/output registers or some other method of clobbering?

In D40334#939939, @craig.topper wrote:

What are you planning to do with 16-bit instructions? Like I said XOR16 is insufficient to break a dependency.

Added subtarget features.
Adding the rr versions of the instructions to hasPartialRegUpdate affected folding of memory operands for these instructions (if an instruction has partial reg update, then it folds only if optimizing for size) - Made a small adjustment the test to recreate the problematic scenario after this change.

Thanks for the comments!
My changes are mostly refactoring of the old ExecutionDepsDix pass in a way that is (almost*) NFC.
This pass combined the logic of both breaking false dependencies and execution domain adjustments in one single pass that traversed the basic blocks in a very special way (optimizing traversal of loops).
I think the first review (https://reviews.llvm.org/D40330) in this bunch would make everything clearer for you.

Fixed vector intialization.
Also incorporated comments from https://reviews.llvm.org/D40333 in this patch, which seems to be more appropriate than the later one.

As compile time was a concern, I've added a flag to control weather we do cost calculations for local intervals we expect to be created (it's on by default for X86 target, off for the rest).
I've fixed the tests and some comments.

Hi Quentin,

Have you had a chance to look at it yet?

As far as I read (http://www.eecs.umich.edu/~mahlke/papers/1993/hwu_jsuper93.pdf), in order to create superblocks, we need to identify traces using execution profile information, and then do tail duplication to avoid multiple entrances.
According the authors of this technique, this transformation as itself takes significant amount of code and compile time.
I don’t think this transformation is something we should do only for the sake of machine copy propagation pass, as it adds significant complexity.
The decision of supporting this transformation and the possible optimizations that can benefit from it seems like an orthogonal discussion that is not directly related to this bad eviction chains I’m trying to solve.

In D35816#841290, @qcolombet wrote:

Based on this I think the solution should probably be kept in Greedy (+ possibly additional cleanup in the copy propagation pass).

Would a super block copy propagation pass work?
I believe the code in that pass should just work in such configuration.

In D35816#822628, @myatsina wrote:

In D35816#819501, @qcolombet wrote:

Hi,

The greedy allocator is already very complicated and I am not sure the additional complexity of the eviction track is worth it.
Is it something that could be cleaned up in machine copy propagation? The problem is very local so that sounds doable.

I will have a closer look to the patch because fixing the problem from the start is obviously better that patching up later, but given how rare that problem is I really believe exploring other, less complex avenue is interesting.

Cheers,
Quentin

Thank you for suggesting the machine copy propagation, I've started working on this direction, it definitely seems easier to implement it there.
On the other hand, if I understood correctly, one of the issues with the old llvm register allocator (linear scan) was that that it did a lot of decisions that the rewriter had to clean up afterwards, and it was intended that greedy will try to avoid such decisions. I'm not sure if this eviction chain falls under this category or not.

Thanks,
Marina

In D35816#819501, @qcolombet wrote:

Hi,

The greedy allocator is already very complicated and I am not sure the additional complexity of the eviction track is worth it.
Is it something that could be cleaned up in machine copy propagation? The problem is very local so that sounds doable.

I will have a closer look to the patch because fixing the problem from the start is obviously better that patching up later, but given how rare that problem is I really believe exploring other, less complex avenue is interesting.

Cheers,
Quentin

Do you have any update on this change?

LGTM

Do you have a new revision for this change?

I've seen some places that do SmallVector<std::unique_ptr<type>, size>. This might be a bit better as it states explicitly that there is no duplication in this data structure.

I'm sorry, I'm getting too lost in this review with the interaction between all the changes and especially trying to check if all the corner cases were tested sufficiently.
I want to make sure we implement each of your changes the best possible way.

The existent tests you've changes seem to be affect by other changes as well.
Can you please upload only the changed tests' logic that is affected by this patch alone?
I would not like to see the affects of "xor fusion" or the other things here, just the support for call instructions in clearance calculation

First, I'm sorry for the late review :)

LGTM

Added a few minor comments.

In D28468#648538, @niravd wrote:

In D28468#648513, @myatsina wrote:

Can you see the tests? I've added comments next to them.
Do this tests capture what you meant?

Yes.

But I thought you also had one using llvm-objdump so we also test the object->instruction disassembly path.

Can you see the tests? I've added comments next to them.
Do this tests capture what you meant?

I've added the test that check for errors.

Added relevant data16/data32+lgdt tests.

In D28468#641450, @niravd wrote:

In D28468#639720, @myatsina wrote:

In D28468#639714, @niravd wrote:

Can you add some disassembler tests?

Do you mean to add tests for the tablegen diassembler change?

Yes. I also notice that the testsuite is short on testing data16/data32. It'd be nice to add a few more tests for these as well.

In D28468#639714, @niravd wrote:

Can you add some disassembler tests?