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.

This sounds good to me. I hope a better solution is developed later, but at least we get rid of the warning.

In D54962#1311537, @uweigand wrote:

Could we not simply add a way for the target to tell TableGen to not define any implicit compositions?

In D50977#1310153, @uweigand wrote:

@kparzysz, does that make sense to you?

In D50977#1309705, @uweigand wrote:

I must have missed the earlier discussion, but I agree with @bjope 's comment earlier that subreg_h32(V0) -> F0S is actually wrong; there should not be any subreg_h32(V0) at all!

Fixed the definition of subreg_l32 in the testcase.

Examine subregister (and composition) actions on all registers when detecting ambiguities. This eliminates the warning from the SystemZ build, but keeps the warning in the subreg_ll32 from earlier comments.

In D50725#1303747, @chandlerc wrote:

We're all still waiting on a fix here....

If Krzysztof can't address this, maybe the code owner for SystemZ (CC-ed I
believe) can take a look?

Thanks!

Thanks for the investigation. What concerns me a bit is that this isn't the only place where this pattern occurs, but hopefully it will be detected if it causes problems. It would be nice to have enough data to file a bug report against stdc++, but for now we can apply this workaround.

Hexagon packets (bundles) have 4 slots, numbered 0..3. Each one of the three instructions (2 x S2_extractu, and PS_call_nr) can only go in slots 2 or 3, so something went horribly wrong.

The encoding of an instruction includes encodings of its operands. How are you planning to implement separate encoder/decoder methods? Right now their names are all embedded into operand definitions.