This is good regardless of any future changes to replace getAsString.

LGTM with a few nitpicks (inline comments). Also, could you commit the DenseSet/tombstone changes in a separate patch?

In D37957#875513, @zturner wrote:

Can you apply this patch Krzysztof and see if you can figure out what the right thing to do is with the empty StringRef?

Committed in https://reviews.llvm.org/rL313362.

Could you rebase this patch?

On my machine, in the debug build of TableGen, the time for X86 -gen-dag-isel went down from 61s to 52s.

On another note, this patch was meant to prevent adding extra branches to the header from within the loop. Branches to the header that come from outside of the loop should be fine. In other words, it should be ok to refine the check to only disable jump threading when any of the PredBBs is dominated by the header (SuccBB).

In D37611#870742, @JesperAntonsson wrote:

I found and fixed the reason our randomized tests didn't run cleanly. It was essentially that stepBackwards removed defined regs from the set even if the instruction was predicated.

Reduced code duplication between the legacy (old PM) passes.

Late jump threading is in D37816.

I'll come up with a late jump threading pass that you can try plugging in, in addition to having the existing one.

Have you tried it? Changing jump threading to have early/late versions is trivial, the question is when should the late one run, and if that would help in the first place.

Good news everyone... The Hexagon test suite passed.

To add to my previous comment---there is a plan to eliminate kill flags in the future (as I was told), so passes the rely on those will eventually need to be modified. If it's the presence of kill flags on predicated instructions (and tied uses) that is causing problems, maybe it would be a good thing to address that in the passes that are exploiting that to generate wrong code. It would imply more work, so I guess the answer depends on how much more work it would be.

I just started a test run on a Hexagon suite, but I wonder if the anti-dependence breaker could be a problem. This is regarding the question of whether the implicit uses on predicated instructions should be killed or not. BTW, the same argument applies to tied uses. Even though the live ranges seem to end/start at that instruction, the register cannot be renamed in only one of them. This differentiates those instructions from a case of unrelated use/def coincidentally using the same register (as was in the example: r0 = add r0, r1).

Addressed Simon's comments.

If you decide that the YAML file doesn't need to be modified in this patch, the rest LGTM.

Should you also change lib/ObjectYAML/ELFYAML.cpp?

This seems like a reasonable way of handling it, but I'd like Matthias and Kyle to comment on this as well.

• Kept the original code for the case of empty set.
• Added the same fix to LiveRegUnits.

Replaced by D37034.

As a side note, in case anybody is wondering: the differences in the generated code come from the post-RA scheduler, which can do more reordering of instructions relative to bundles.

Changed the code to simply disable jump threading into loop headers.

This is great. Thanks!

On a somewhat unrelated note---is it valid on AArch64 to reserve W1_W2 without reserving W1 and W2? If so, it would lead to a situation where a reserved register does not contain any reserved units. If not, it would require one or both of W0_W1 and W2_W3 to also be reserved.

Are there any new developments?

Can this review be closed?

Rebased.

Any thoughts on this?

The Hexagon changes look good to me.

Ping.

Looks good to me. One thing I'm wondering about is if this needs to be a separate TableGen "target". Could it be under a debug flag used with -gen-register-info?

Please make sure that the copyright notices are ok with everyone. Other files don't have them and I don't know whether that works with the current license/legal status.

I found a few minor things, with those changes it looks good to go to me.

Ping.

The problem is that the mismatch between sections does not have to lead to any undesirable behavior. Whether it does or not depends on a particular case. I think we should be consistent though---if we decide that a mismatch between the section for a declaration and the section for a definition leads to an undefined behavior, then it should be so regardless of whether the sections were given explicitly or inferred.

In D36712#843387, @eandrews wrote:

Does this result in unexpected behavior though? Won't this just result in the global being defined in the specified section?

In D36712#842433, @efriedma wrote:

I can't see how "undefined behavior" could possibly be the right answer in that case. Every definition has to end up in some section eventually, and in many cases we don't know what section that will be when a global is declared. (For example, we put constants into different sections depending on the contents of the initializer.)

Ping.

Changed the type/variable names to something I find more accurate.

Intrinsics are not handled here because their types are exposed to outside of TableGen, specifically to the front-end. We have two sets of HVX intrinsics on Hexagon, one for 64-byte and one for 128-byte mode, and they are not going away. The rest of clang/LLVM would need to be changed to deal with that. I've tried to handle intrinsics too, but quickly realized that it wouldn't be possible to have that done within the scope of TableGen.

Added comments as requested by Matthias.