Is this still planned to be checked-in?

The revert also highlights the problem that we don't have representative test in llvm/test :(

In D87674#2290419, @arsenm wrote:

In D87674#2290418, @madhur13490 wrote:

In D87674#2290396, @mareko wrote:

Yes, this commit is incorrect. It completely breaks code linking in Mesa OpenGL. s_waitcnt is required at the end of all global functions that return values.

Please revert. @nhaehnle

I don't understand why would it fail. This patch just moves s_waitcnt to the caller so they would be executed anyway. I think I am missing something. It would be helpful to root cause if we can isolate to a small test case.

Shader returns aren't real returns and the "caller" doesn't wait

In D87674#2290396, @mareko wrote:

Yes, this commit is incorrect. It completely breaks code linking in Mesa OpenGL. s_waitcnt is required at the end of all global functions that return values.

Please revert. @nhaehnle

A lot of clang-format errors still exists. Looks okay otherwise.

In D87393#2271038, @Paul-C-Anagnostopoulos wrote:

@madhur13490 I simply copied the preceding setValue() function. Let me commit this the way it is and then I will revisit both functions.

I will capitalize the induction variable.

I can't access the link in the description of the change. Moreover, I don't understand the purpose of making several calls to DTraits.renderNodeUsingHTML(Node). In its current form, the code queries just once and uses the value throughout the code, with your change you'd be calling several times. If this is a pre-requisite to some other change then I'd like to see that change so understand the whole picture. (Phabricator allows you to link revisions and even create a dependency FWIW.)

Reviewing. Sorry for the delay, need a day or so.

In D86203#2230218, @ehjogab wrote:

In D86203#2230217, @madhur13490 wrote:

In D86203#2229816, @ehjogab wrote:

In D86203#2226535, @arsenm wrote:

In D86203#2226357, @ehjogab wrote:

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either.
e.g.
RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.

In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.

The operands of these VS_* instructions, are the defined (in the instruction) to use the VS_32 class, or VGPR_32/SGPR_32 classes? If it is the former, then what this patch would do is to use VS_32 in the patterns. If the patterns should really be using VGPR_32/SGPR_32, then this is a problem. However, you still have the option of annotating the pattern operands just as before, so this patch would not interfere with any patterns that today are accepted by TableGen. If it is the latter, then this patch would use whatever class (VGPR_32 or SGPR_32) that is specified for the corresponding operands in the instruction, so here I don't see there being any problem.

Perhaps we could add an option that this feature is disabled by default, and by activating it you take responsibility to ensure that you don't have these kinds of situations for your target?

When a register class is an union of two or more classes then such problems bound to occur for one or more targets. Why not do such inference when it is obvious i.e. register class in instruction definition is a leaf register class and NOT an union of some base other base classes. I think this is better than having an runtime option to enable/disable the inference.
I think leaf classes can be detected programmatically.

I like that idea, and this is only applied on leaf nodes to begin with.

In D86203#2229816, @ehjogab wrote:

In D86203#2226535, @arsenm wrote:

In D86203#2226357, @ehjogab wrote:

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either.
e.g.
RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.

In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.

The operands of these VS_* instructions, are the defined (in the instruction) to use the VS_32 class, or VGPR_32/SGPR_32 classes? If it is the former, then what this patch would do is to use VS_32 in the patterns. If the patterns should really be using VGPR_32/SGPR_32, then this is a problem. However, you still have the option of annotating the pattern operands just as before, so this patch would not interfere with any patterns that today are accepted by TableGen. If it is the latter, then this patch would use whatever class (VGPR_32 or SGPR_32) that is specified for the corresponding operands in the instruction, so here I don't see there being any problem.

Perhaps we could add an option that this feature is disabled by default, and by activating it you take responsibility to ensure that you don't have these kinds of situations for your target?

In D83034#2224104, @arsenm wrote:

LGTM. I think this is only a half solution, and it's still a problem that multiple builtin predicates will be silently ignored

fix unexpected tabs

fix formatting

rebase + address comments

@arsenm , Can you please take over? I think @dsanders is on vacation.

reviewing.

rebase + address comments

In D84391#2170772, @scott.linder wrote:

In D84391#2170741, @scott.linder wrote:

Can we just add a predicate for whatever necessitates the SETREG for gfx10+? No idea on a good name, but if it were flatScratchIsHwreg() and it implied flatScratchIsPointer() this becomes:

if (ST.flatScratchIsHwreg()) {
  // pointer add then S_SETREG
} else if (ST.flatScratchIsPointer()) (
  // pointer add
} else {
  // size and offset dance
}
llvm_unreachable();

I agree that the existing assert here doesn't really mean anything, not sure why I wrote it.

Rather, the llvm_unreachable isn't needed if we consider the offset/add case as the "default", but maybe we should predicate it too? It would be nice if we could do this all in a way that was statically known to cover all the targets we have.

Could we combine the various predicate functions into one which returns an enumeration instead? Similar to Madhur's proposal, but without the direct reference to GFX#? I.e.

switch (ST.getFlatScratchSetupKind()) {
AMDGPU::PointerInHwreg:
  // ...
  break;
AMDGPU::PointerInSGPR:
  // ...
  break;
AMDGPU::SizeAndOffsetInSGPR:
  // ...
  break;
}

Then when we add a new variant the compiler points us to everywhere that cares.

rebase

In D84391#2169493, @arsenm wrote:

In D84391#2169473, @madhur13490 wrote:

I am not sure what does that mean. Do you agree with my way of refactoring or you're suggesting something else? With my approach there won't be any checks as I said we can get rid of flatScratchIsPointer() too.

I mean exactly the opposite. We should not check the generation anywhere. The hardware does not make changes in a nice, disciplined, sequenced way. Everything should be based on specific feature checks. I.e. there should only be flatScratchIsPointer checks, and nothing checking getGeneration

In D84391#2169391, @arsenm wrote:

In D84391#2169292, @madhur13490 wrote:

In D84391#2169172, @arsenm wrote:

Needs test. Also not sure why we would have an assert for this here in the first place

In fact I like the assert. It guarantees that we're not building something insane. Moreover, should we refactor this code to something more readble? :

switch(llvm::AMDGPUSubTarget) {
case GFX:8 {}
case GFX:9: {}
...
}

Only caveat is the usage of function flatScratchIsPointer(). We can get rid of its usage in its current form but in future we may want to add some code to it to take contextual decisions.

I would like to purge all checks of the generation. The relevant bit is flatScratchIsPointer. The generation is redundant and potentially misleading.

In D84391#2169172, @arsenm wrote:

Needs test. Also not sure why we would have an assert for this here in the first place

LGTM

In D83034#2133027, @arsenm wrote:

I would expect the existing tablegen tests to break from this and need updating?

rebase + add test

In D83034#2133989, @arsenm wrote:

In D83034#2133375, @dsanders wrote:

Could you add a test case or point us at a example of an existing test that goes wrong? The description in the commit message is unlikely to be the full story as we already cover loads with non-null MemoryVT's. My best guess is you are attempting to use builtin predicates and custom predicates together. I don't see a reason why that shouldn't be allowed but it's not something that was intended as the goal was aiming to fully remove the custom C++ from the tablegen headers so that tablegen could do some transformations on sextload/zextload and similar to fix DAGISel vs GlobalISel modelling mismatches.

I think the problem is broader than just combining custom predicates and builtin. The emitter here implicitly assumes all of these builtin PatFrag predicates are used exactly as the hierarchy used to define the default/generic load/store patterns. However, the PatFrags are much more free form and you can define a patfrag that combines multiple of these predicates in the same "layer" of the load/store hierarchy. The AMDGPU patterns redefine the entire set of load/store patterns, and in some cases it makes sense to combine all the predicates at once. I had to somewhat artificially add new layers of PatFrags to only apply a single predicate at a time. This also isn't consistently applied (for example, it does work to combine the AddressSpaces predicate and alignment)

Ping!

Fix test failures

rebase + address comments

@jlpeyton Any idea why the tests you added may be failing? I don't think it has anything related to my changes.

fix tests

rebase

remove redundant print

Address comments by arsenm, revise description

In D81806#2095487, @simoll wrote:

In D81806#2094960, @sameerds wrote:

In D81806#2094869, @simoll wrote:

I am a bit surprised this is necessary. Actually, join divergence in bb3 should come about as a result of propagateLoopDivergence for the [bb1, bb2] loop and not immediately because it is a divergent exit.

I am not sure what you mean here. propagateLoopDivergence is calling propagateJoinDivergence ... so doesn't that count as a result of the former?

Also, a divergent loop exit does not automatically become join divergent, eg:

bb1: 
   %m.uni = <uni>
   %n.uni = <uni>
   br <uni>, %bb2, %bb3

bb2: [..]
   br <div>, %bb2, %bb3

bb3:
   %x.uni = phi [%m.uni, %bb2], [%n.uni %bb1]  <-- divergent loop exit that is not join divergent

But here, bb3 is not a join block for the bb2 loop, since there is only one path from the loop exit reaching bb3.

Exactly. With this patch %x.uni is marked as join divergent even though it is uniform: In its present form the patch solves one issue at the cost of precision.

The real culprit here is the SDA::join_blocks interface that does not distinguish divergent join blocks and loop exits. The SDA "knows" which is which internally, loses that information as it passes divergent blocks through join_blocks and the DA has to reverse-engineer that information. I have an unfinished patch here that fixes that and should resolve the bug addressed by this patch as well. The unfinished patch also factors the back-and-forth between the DA and SDA to propagate along nested loops completely into the SDA (propagateLoopDiv does not really belong in the DA).

Long story short: This is an ok fix (but please add a FIXME/TODO that hints to the issues with this approach) but we should rework the join_blocks interface in any case.

rebase

Address further comment by arsenm

Fix clang-tidy errors

Addressing comments by arsenm

Update summary