Looks good to me.

The Hexagon part looks ok.

To clarify: clang-8 hangs on HexagonInstrInfo.cpp when compiling current master branch with this patch applied.

This seems to hang when compiling HexagonInstrInfo.cpp.

LGTM.

What is this in preparation of? Why are only these 3 tests affected?

This commit stopped clang++ from finding libc++:

Ping.

Ping.

The problem is that in some contexts the explicit sign- or zero-extension may be deliberate, regardless of how boolean values are represented in wider types. Other than delegating that to the target experts, I don't know how to address that.

The Hexagon part is ok.

This commit seems to be implicated in a cmake-time warning. See http://lists.llvm.org/pipermail/cfe-dev/2019-March/061837.html for more information.

The Hexagon testcase can be fixed---it's probably just a matter of changing the selection pattern for the instruction we're checking.

There can be many changes to the compiled code between the IR combiner and the DAG combiner, so these patterns can certainly appear before DAG combining takes place. Also, we already combine for rotates in the DAG, this patch only makes it more comprehensive.

Rebased on top of the pre-committed testcases.

Oof, I somehow missed the comments... :o

The Hexagon changes look good.

I was actually thinking about handling of target-specific relocations, and applying them to instructions. We used to have a large chunk of auto-generated code that did that. The implementation in lld is much shorter, so it's probably not a big deal if it needs to be repeated.

Since you're rewrite this, is there any potential for sharing code with lld? Relocating instructions should essentially be the same between these two.

Looks ok to me.

Ping.

This patch uses MatchRotate to do the actual matching, so it's only going to look for opportunities to create a rotate. Maybe that function should be replaced with MatchRotateOrFunnelShift in a future patch.

Are you opposed to having this done in the DAG combiner?

The Hexagon changes are OK.

One goal was to be able to generate rol-and-accumulate instruction (on Hexagon), specifically for the accumulate operation being | (see f11 in rotate.ll). For the C code we still don't generate it:

unsigned blah(unsigned x, unsigned s) {
  return s | (x << 27) | (x >> 5);
}

I see. Thanks for looking into it.

I'm not expecting this patch to be complete. Most of the intrinsic definitions are actually auto-generated, so we'll need to update the generating scripts to include these new annotations. What concerns me is that you claim that some things are missing, which could indicate that there are some issues with the generators. I'm trying to understand exactly what the missing pieces are, the contents of this patch are actually of secondary importance.

In D58356#1401321, @arsenm wrote:

There has to be a corresponding builtin definition explicitly defined in clang, which also needs to correctly mark the required constant operands. The test coverage for which operands need to be constants is spotty or nonexistent for these, so those are possibly missing in the builtin definition.

I don't really understand the summary, could you elaborate on how the asserting implies the conclusion (the part that follows "This means that..."). Also, what does that part mean on its own? "Intrinsics are missing that don't use GCCBuiltin...", meaning that you'd expect some intrinsics that don't use GCCBuiltin, but they're not defined?

I'm not trying to block this from being committed, but I'm really not a fan of it. I think that register units are fundamental enough not to be treated as an "implementation detail". Besides, they are quite explicit in the public interface of LiveIntervals.

In D57608#1381719, @aemerson wrote:

If we're going to add a helper to shave some characters off, we might as well go for something really short like getOpReg().

The Hexagon tests were meant to check operations on vector predicate registers, so for them to work the inputs need to be vectors of i1 with the lengths that correspond to the lengths of vectors of 8+ bit integers. The only way to generate such values is do either a compare or a truncate. Since compare instructions can be fused with logical operations, this leaves truncate as the only option. With the changes from this patch, truncate is also eliminated, so it appears that these instructions cannot be emitted anymore (from a non-intrinsic code). With this in mind, I am ok with these changes.

I've run some tests and the results are not great for us. On some tests we got up to 5.5% improvements, but there are a lot of severe degradations (15+% worse). If this patch goes in, we'd like to be able to opt out.