Hi Valery, I really like the way the different cases are listed in the explanatory comment at the top of the file, and I believe those cases are correct. Would it be possible to restructure the code in a way that follows those cases? I think that would make it much easier to follow.

LGTM

The only user of canReadVGPR is addUsersToMoveToVALUWorklist. Since the intended semantics of canReadVGPR aren't at all clear from the name, might I suggesting folding it into its only user?

Thanks, LGTM

LGTM

LGTM

In D55369#1321530, @arsenm wrote:

I thought mesa was moving to stop using the relocations at all for this?

Please make the change apply to Mesa3D as well.

Note, we still have one use of this in Mesa.

LGTM

Can we also remove the setTruncStoreAction mention of v16i8 from SIISelLowering?

LGTM

LGTM

LGTM

Oops, I thought I'd posted this earlier... LGTM.

In D53493#1307611, @simoll wrote:

In D53493#1292733, @nhaehnle wrote:

LGTM. Do you need me to commit this?

Yes, i cannot commit myself.

What are the current future plans for this whole series? I think we should enable this by default, but it would be nice to get rid of the old code, which requires addressing the irreducible test case. I vaguely recall that you mentioned a way to address that, but I can't find that now.

I will open a new patch series to address the irreducibility issue. That series should also involve deprecating the LegacyDivergenceAnalysis for good.

Sad, but LGTM

One last thing. If I'm right about this, it'd be good to reduce the check complexity and indentation levels. Then I'm happy :)

• prefer to use !TRI.isSGPRReg

LGTM

Should there be a change to LangRef?

FWIW, I also think it's important not to drop MachineMemOperands. As Hal points out, MMOs carry non-optional information such as volatile-ness. And the Size is non-optional information at least for the AMDGPU backend.

Thank you. One question left though.

ping

ping^4

This should already be fixed in trunk. Trunk has a function TII->hasUnwantedEffectsWhenEXECEmpty to cover this, and this patch simply shouldn't apply.

• add TODO comment about the opportunity of keeping per-event timelines

• Early-out in generateWaitcntInstBefore when no wait is needed. This helps keep the nesting complexity in check for later changes.

ping^3

ping

I think the remarks by @t-tye point to a potentially useful optimization, but that should not be part of this patch.

ping

Yeah, the separate DAG combine for scalar select w/ undef is a better solution. LGTM.

In D54606#1301212, @rampitec wrote:

However, why does code with undef vectors look so bad? For example, in float4_inselt, the fact that the initial vector is undef should allow us to just store a splat of 1.0.

Yes, I noticed that too. That needs to be a separate optimization. As far as I understand "insert_vector_element undef, %var, %idx" should not even come to this point. It needs to be replaced by build_vector (n x %var) regardless of the thresholds and heuristics I am using, e.g. earlier (higher in the same function I think).

In D54516#1301197, @sameerds wrote:

In D54516#1301072, @tpr wrote:

EarlyCSE does seem to common up in this situation. And, if I disable that, I get GVN commoning it up.

By "disable", do you mean modifying EarlyCSE to not touch convergent calls? What if you do that for GVN as well? GVN::ValueTable::lookupOrAddCall() seems to be the right place for that. Such spot fixes as a useful step in the right direction, to be preferred over repeating the asm hack. One already exists in GVNHoist. This may sound a bit whackamoley, but the "effort" that @nhaehnle was asking about is essentially a more organized way to audit all the places where convergent calls should be handled specially. That project has not gained sufficient motivation yet to commit to.

In D54516#1301013, @cwabbott wrote:

In D54516#1300997, @nhaehnle wrote:

In D54516#1298178, @cwabbott wrote:

I believe the combination of Convergent + not Speculatable should mean that the compiler shouldn't hoist it to a non-control-equivalent block and shouldn't CSE it. In particular, IIRC it's not guaranteed that that a readnone function always return the same value when it's called with the same arguments, so it's not safe to CSE -- it just means that LLVM can move other things across it, since it doesn't modify *caller-visible* state. What pass is causing a problem? Maybe it's a bug in the pass?

readnone is literally readnone. Maybe you're mixing this up with inaccessiblememonly?

No, I meant readnone, since AFAIK that's what IntrNoMem here maps to. The bit about "caller-visible state" was taken directly from the langref entry for readnone. My point was that in something like:

WWM code
if (cc) {
  other stuff v1
} else {
  identical WWM code
  other stuff v2
}

it wouldn't be allowed for LLVM to rewrite the use of the second WWM to point to the first, even now, since the semantics of readnone aren't strong enough to guarantee that the two calls return the same thing, at least as far as I understand (I think someone else, maybe Tom, explained this to me over IRC a while ago). Your issue is different, and indeed removing IntrNoMem isn't going to help with that at all. I agree that we need a better solution for that. But for the current patch, I can't think of a situation where removing NoMem/readonly would disallow a transform that shouldn't be allowed.

Thank you, this looks much cleaner. I only have a small number of nitpicks left.

In D54516#1298178, @cwabbott wrote:

I believe the combination of Convergent + not Speculatable should mean that the compiler shouldn't hoist it to a non-control-equivalent block and shouldn't CSE it. In particular, IIRC it's not guaranteed that that a readnone function always return the same value when it's called with the same arguments, so it's not safe to CSE -- it just means that LLVM can move other things across it, since it doesn't modify *caller-visible* state. What pass is causing a problem? Maybe it's a bug in the pass?

I agree this needs a test case.

In D54340#1293411, @arsenm wrote:

Is there code for the lazy map that should be cleaned up now?

• add LLVM_ATTRIBUTE_UNUSED to prevent warning in optimized builds
• cleanup isSDNodeSourceOfDivergence a bit more

Mostly looks good to me.

LGTM

The update seems to have messed up the indentation of comments in a few places.

Note, the new waitcnt-preexisting.mir test shows this change.

Turns out I was a bit too quick in my analysis of the second point.
I thought the overly conservative waitcnt was due to the control flow
in the shader I was looking at, but it was actually due to a pre-existing
waitcnt.

In D54228#1290997, @t-tye wrote:

This is sufficient, because whenever only one event of a count type is

pending, its last time point is naturally the upper bound of all time
points of this count type, and when multiple event types are pending,
the count type has gone out of order and an s_waitcnt to 0 is required
to clear any pending event type (and will then clear all pending event
types for that count type).

Just wondered if can do better than using 0. Instead can the lowest count be used as this should be sufficient to ensure all out-of-order events in this have happened? I had discussed this with Bob at one time.

LGTM. Do you need me to commit this?

LGTM

The code change looks fine to me, but it should be possible to cleanup the test case a bit.

Two stylistic nitpicks. LGTM with those addressed.

The huge switch statements are a poster child for the generic SearchableTables, somewhat analogous to what already exists for MIMGInstructions. Sketching it out:

class LoadStoreBaseOpcode {
  LoadStoreBaseOpcode BaseOpcode = !cast<LoadStoreBaseOpcode>(NAME);
  bit Srsrc;
  bit Sbase;
  ...
}

In D53283#1282151, @hakzsam wrote:

Hi Nicolai,

Fyi, This introduced a regression with Mass Effect Andromeda with DXVK and RADV on Polaris10. See https://bugs.freedesktop.org/show_bug.cgi?id=108611

In D54153#1288936, @rnk wrote:

In D54153#1288824, @aganea wrote:

The issue is that XXX-tablegen-host .vcxprojects are explicitly calling cmake --build, thus not going through MSBuild's dependency graph. It looks like the Ninja generator doesn't do that.

Really? I thought it did... The main reason I don't use the "optimized tablegen" build configuration is because it doesn't keep everything in a single ninja build file. I just always compile with optimization, asserts, and debug info enabled.

Interesting. Thanks for digging into buildsystem stuff!

Thanks for making those changes. The EOF handling does look better.

As an overall algorithmic remark: I like the organization of the pass into phases, because it provides a path forward to an additional optimization.

In D53840#1282880, @vzakhari wrote:

In D53840#1282024, @nhaehnle wrote:

I have to say I'm feeling a bit ambivalent about this. I'd say it would be nicer to have a mechanism that integrates with the rest of the TableGen language, but that's admittedly non-trivial. So I guess this approach is okay.

Thank you for the prompt reply, Nikolai! Yes, I decided to keep the preprocessing aside to minimize changes in the actual lexing.

The handling of end-of-files is a bit wonky. Have you considered just returning EOF from getNextChar at end of file, even if there's a parent file, and changing the EOF case in LexToken to just loop (or tail-recurse, I suppose) if it was the EOF of an included file?

This is possible, though, EOF handling will be required in SkipCComment() and LexBracket(). If you agree with me changing these routines, I can do this. Returning EOF from getNextChar() allows handling cross-file C-style comments and bracket contructs - I can disallow these use-cases or keep supporting them. What do you think I should do?

In D53496#1282412, @hakzsam wrote:

This regresses the following tests on RADV:

dEQP-VK.glsl.loops.special.for_uniform_iterations.select_iteration_count_fragment,Fail
dEQP-VK.glsl.loops.special.for_uniform_iterations.select_iteration_count_vertex,Fail
dEQP-VK.glsl.loops.special.while_uniform_iterations.select_iteration_count_fragment,Fail
dEQP-VK.glsl.loops.special.while_uniform_iterations.select_iteration_count_vertex,Fail

LGTM

Thanks for the heads up. I'll take a look.

In D53496#1277330, @alex-t wrote:

It seems like we have to further develop this approach to deal with the scalar comparison instructions.
For instance, S_CMP_* does not produce any result but implicitly defines SCC.
Thus, InstrEmitter will insert the copies all the time.
Since DAG operator SETCC produces i1 value there will be the SCC to VReg_1 copies.
I not trying to invent a method to lower that copies.
First issue: in case all the uses are not divergent I don't need the V_CND_MASK -1,0 -> V_CMP_NE 0 pair
I need S_CSELECT -1, 0 immediately after the definition (to save SCC) and S_CMP_NE 0 just before use to rematerialize SCC
Second issue: I only need to save/restore if there are SCC defs in between.
So, we need to take into account not divergent flow as well.

Add missing V_XAD_U32 pattern.

Thanks for making the changes.

I have to say I'm feeling a bit ambivalent about this. I'd say it would be nicer to have a mechanism that integrates with the rest of the TableGen language, but that's admittedly non-trivial. So I guess this approach is okay.

Yay for better error messages! One comment about unfortunate variable naming...