LGTM, with one minor nitpick.

LGTM

LGTM

LGTM

Test case? LGTM apart from that.

LGTM

This makes sense, but I do have one comment.

Don't we *want* clusters that large, and even larger?

Once the form of the attributes is finalized, frontends can start emitting the new form and we just won’t look it it until this patch

I agree with Hal that if/else should introduce a scope. Otherwise, you get into consistency problems. Consider:

multiclass Foo<...> {
   if ... then {
      defvar bar = ...;
   }
   defvar baz = bar;
}

Without introducing a scope, the above is presumably valid but in a rather surprising way. And a similar example with an else that also defines bar wouldn't be valid due to the redefinition.

In D71407#1781506, @simon_tatham wrote:

• should a defvar in a foreach be local to it? (Probably)
• should I allow defvar inside a class or def as well as in a multiclass, defining variables that vanish at the closing brace but can be referred to by the definitions of proper class member values? (Possibly)
• is defvar the sensible name? (def and let were already taken; I considered C++ using, but thought defvar fits reasonably well with the existing defset)

In D71213#1781322, @gchatelet wrote:

LLVM has this LLVM_ATTRIBUTE_DEPRECATED macro, it's convenient to get a warning but it only works when building without -Wall.

LGTM

Thanks for the heads-up. I think this is perfectly fine from an AMDGPU perspective. It also seems like a simple oversight that the ExternalAAWrapperPass is not in that list, so this LGTM in general, but I'd wait a bit for somebody who is more familiar with alias analysis to weigh in.

Would it be possible to do this patch in two steps:

This basically LGTM

As a general rule, I think it would be preferable for patches such as this one to be split up further, especially when they touch common code. For example, why does a patch by the name "Target vector intrinsics" contain a change to SjLjEHPrepare?

Okay, digging in further, the feature is actually used quite a bit in pattern definitions like:

def : GCNPat <
  (VGPRImm<(i32 imm)>:$imm),
  (V_MOV_B32_e32 imm:$imm)
>;

or

def: Pat<(s32_0ImmPred:$s16), (A2_tfrsi imm:$s16)>;

The first really ought to be expressible by placing the $imm inside the inner DAG expression, but for some reason the TableGen backend doesn't allow that. I don't know about the second... maybe if it was wrapped as (i32 s32_0ImmPred:$s16)? Fundamentally, the feature seems to be used as a hacky workaround for the fact that SelectionDAG patterns don't have an "outer" DAG, and having a naked expression like s32_0ImmPred:$s16 is not possible.

The name suffix _can_ be applied to the operator:

def op;
def Foo {
  dag x = (op:$blah 1:$foo, 2, 3);
}
// produces:
------------- Defs -----------------
def Foo {
  dag x = (op:blah 1:$foo, 2, 3);
}
def op {
}

Though the inconsistent output and undocumented nature of it suggests that this feature is hardly ever used...

My concern is that there's something that's going to blow up or miscompile if we start passing in constants that aren't in a regular address space. Aren't there kinds of annotations which get persisted into the emitted code?

This looks fine, but have you thought through !setop discarding the name on the operand? The only use of this feature seems to be naming DAG pattern nodes, a feature that seems to have been around forever but I haven't seen used in the code I've touched...

LGTM

Thanks, LGTM

LGTM

In D71208#1776202, @rjmccall wrote:

The questions I'd like to have answered before I can approve this are:

• whether there are clients of @llvm.global.annotations that will have problems with non-0 address spaces and

This mostly looks good, except I strongly suspect that all other export intrinsics should have their done bit set to 0 in this case.

I don't feel particularly strongly either way, but it seems to me that allowing an unset operator could perhaps be the most elegant solution after all. I'm lacking the context of your particular use case, but it seems to me you're attempting to piece together DAG fragments in a generic way where the operator simply isn't known yet by some of the pieces of TableGen you're writing, and using an unset operator could be a more natural way of expressing that than coming up with some artificial operator that is simply thrown away later.

With this change, it looks as though accumulateUsedDefed is called multiple times on some paths. That seems wrong. It does seem plausible that we'd have to keep accumulating for all code paths.

Thanks. This basically looks good to me, some minor nitpicks.

Thanks for doing this, LGTM.

As a general rule, I think we should avoid unrelated whitespace changes, of which this patch has a few, because it can make merging/cherry-picking/rebasing/blaming more confusing. If you're using clang-format, a good way to achieve this is to pipe the output of git show -U0, git diff -U0 or similar into clang-format-diff -p1. It's acceptable here though.

ping^2

Thank you for your detailed comments!

• add some more expository text and examples
• handle the case of speculatable+convergent

On the issue of a test case: I don't think it's practically possible to hit the crash I was worried about right now, because AddrIdx[0] happens to be small enough in all cases that it will be a valid operand index in all cases, and then hasSameBaseAddress will bail out for machine instructions of a different class because the register comparison of the first index will fail. However, that's just an artifact of how machine operand indices happen to be laid out today. I still maintain that the change itself is the right way to do things.

In D68994#1724647, @kariddi wrote:

What is the expectiation of lowering of a loop like the one you mentioned above?

int divergent_key = ...;
int v = ...;
int sum;

for (;;;) {
  tok = @llvm.convergence.anchor()
  int uniform_key = readfirstlane(divergent_key);
  if (uniform_key == divergent_key) {
    sum = subgroup_reduce_add(v);
    @llvm.convergence.point(tok)
    break;
  }
}

In particular the expectation of lowering in LLVM-IR is typically that the block with the "break" is an exit block and as such is typically moved outside of the loop (when the threads are reconverted). Is the expectation to have LLVM.CONVERGENCE.POINT to be lowered to the backends with a pseudo instruction similar to DBG_VALUE (and then be ignored by regalloc and such) up to the point that final control-flow block ordering is defined so that the execution of the "unconverged" part of the exit block is lowered so that is executed inside the loop instead of outside?

As you know, I am very much in favor of this change, and really anything that re-establishes the rule that code is treated maximally conservatively when there are no attributes or metadata.

LGTM

LGTM

Right, I don't think RPO hurts, but I also didn't (and still don't) see how it would help due to the possible circularity between PHIs, so I'd rather stick to the simpler iteration. Otherwise somebody looking at the code later may think the RPO is important for some aspect of the algorithm.

LGTM

Thanks. Any thoughts on the higher-level algorithmic questions?

LGTM

I haven't looked into the set of instructions that are effected by this, but the TableGen LGTM.

In D68994#1709802, @arsenm wrote:

Are you planning on adding codegen support in a separate patch?

Yeah, it doesn't seem very relevant for the element argument, though precisely because of that I'd feel more comfortable with ANYEXT.

Thanks for tracking that down. I agree with Stas that SmallSet should be a better choice here. Also some minor comments on the tests, apart from that looks good to me.

LGTM

Thank you for persisting through this.

In D68690#1701685, @arsenm wrote:

Testcase?

Mostly LGTM, but I wonder about the high level intention here. Is this intended to expose new load/store merging opportunities? If so, is there a test for this? Or is there some part of SIFoldOperands that can now be removed?

I think the code would benefit from the refactoring I've mentioned on the other patch, where the lists only hold a structure with information on a single instruction. Maybe call it CandidateInfo (information of one instruction, persistent in lists) vs. CombineInfo (information on a pair, only temporary).

Thank you for doing this, it seems quite useful. As a follow-up to this change, do you think it makes sense to refactor CombineInfo a bit? We have a list of mergeable instructions, but the CombineInfo structure also has fields for a second instruction, which are only for temporary use, which is a bit odd.

I do have one question. Apart from that it LGTM.

LGTM

I think this should be good to go.

This does seem like a useful addition (heh) to the grammar. There is one thing that can go wrong in the future: parentheses are already reserved for DAGs. Brackets and braces also already have their own purpose. We could designate (( without space for parenthesis, with the risk that ( ( and (( can mean different things, with the first one making sense in DAGs. Though DAG operators are usually defs, so the risk of weirdness may be acceptable.

Thanks, this makes a lot of sense. LGTM.

Huh, interesting. I guess nobody had the time to really dig into why. Anyway, thanks. I think it'd be good to put this kind of information into our commit messages.

For the vector itself and the inserted element, shouldn't this be using G_ANYEXT instead? Looking at the corresponding test, the G_SHL/G_ASHR on $vgpr0 should be unnecessary based on the original code.

What's the reason for having this flag?

LGTM

Use getRegStateUndef

FWIW, the reason that radeonsi uses amdgcn-- is the scratch buffer ABI.

LGTM, with two nitpicks inline.

Thank you for working on this.

LGTM, with two nitpicks.

LGTM

This is a useful change, but there is an unfortunate asymmetry here in how the code is structured: in addition to extractelement, we could also have shufflevector users (or masked stores etc.). Presumably we'd be able to handle all of those together without duplicating the code. Is there a way to take this into account?

Hmm, those test changes were unexpected.

LGTM

LGTM

Sure, why not.