I figured it would be a little easier if I looked at these cases by myself. It turns out there are more problems with isIdentityValue, including some correctness issues. After fixing these, everything works correctly now.

Sorry, I just got back from break this week. I've run CTS with the pass enabled, and it now passes, although it seems most of the patterns we use don't get folded. Firstly AND, XOR, unsigned max, and unsigned min are most troubling, since the code that gets generated looks like it should be optimized:

In D55314#1321326, @vpykhtin wrote:

Ok, I'll disable it. I'm not sure about 3rd point: are you sayng the pass doesn't actually perform the optimization or it's fundamentally wrong? Because it implemented to handle "identity" cases for add, mul and min/max.

In D55314#1321222, @vpykhtin wrote:

In D55314#1321167, @hakzsam wrote:

Hi,

This change breaks most of the subgroups tests with RADV (ie. dEQP-VK.subgroups.arithmetic.*).

Any reasons why you enabled it by default? Looks like it now triggers a new bug in the AMDGPU backend.

Thanks!

Hi,

this is awaited feature and other reason is to detect a situation such yours. I think it's better to turn it off and reproduce the failures: how can I do this?

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?

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?

In D46470#1088984, @tpr wrote:

You made a comment in this code that "this is a workaround anyways until LLVM gains the notion of predicated uses and definitions of variables". How do you see that working? Are you aware of anyone else thinking along those lines in the LLVM community? Something to think about for the future.

For the liveness issue, maybe a better way to solve it would be to add a new ENTER_WWM pseudoinstruction similar to EXIT_WWM, and add a matching implicit def to the matching ENTER_WWM whenever we insert an implicit use on EXIT_WWM, and mark both of them as kills. After all, any affected registers only need to interfere with the instructions run in WWM, so that should help with code quality too. I'm not sure why I didn't do that in the first place.

Ping on this one. This is the last outstanding patch for implementing AMD_shader_ballot in Mesa.

It looks like Sam has worked a lot on the assembler, including adding support for DPP instructions, so I'm adding him for the assembler bits. I'd like to get this in before I leave next week, though.

Remove spurious change to AMDGPUAsmParser.cpp

Fix assembling DPP instructions. Also, adopt a more conservative version of
D34715. In particular, we ignore Constraints/DisableEncoding from the original
instruction for the DPP version. The only instruction with any special
constraints is MAC, because of its fake third source, and there it doesn't make
sense to keep the fake third source since it has to be the same as the normal
"old" source anyways. We can revisit this if something else comes up, but I
think this is a good plan for now.

I realized today that this change is bogus, since these lines aren't copying the Constraints from the Profile. They're copying the constraints set via the pseudoinstruction, and removing these lines breaks a few things, in particular assembling MAC instructions, and probably liveness tracking for them too. Originally the changes in D34716 wouldn't work without this, but I'm going to update D34716 with a better solution that passes all the tests.

Rebase on master.

Remove unneccesary stuff from fix-wwm-liveness.mir, based on Matt's comment for wqm.mir.

Fix one last style issue.

Clarify what the code that implements the WQM semantics is doing.

• rename dest of EXIT_WWM to be consistent

• fix style issues
• add test for WWM with integers
• remove unneccesary stuff from MIR test

Ping.

Fixup lowerCopyInstrs() after using setDesc() by removing extra arguments.

Remove leftover $wqm_ctrl.

Rebase on using s_or_saveexec_b64 for WWM (fix test).

• Fix style issues.
• Rebased on using setDesc() in lowerCopyInstrs()
• Reworked logic for determining safe points to transition where SCC isn't live, to make using s_or_saveexec_b64 for entering WWM possible.
• Added tests for WWM across multiple basic blocks and correct SCC handling. For the latter, I couldn't figure out how to make an IR test that exercised that path, so I made a MIR test for it.

Reset to previous version. Whoops.

Whoops, I accidentally squashed this with D35524 while rebasing. Those changes were meant to be applied to that.

Fix WWM SCC test to actually test what it's supposed to, and fix a bug caught by it. Whoops.

• Fixed style issues.
• Rebased on using setDesc() in lowerCopyInstrs()
• Reworked logic for determining safe points to transition where SCC isn't live, to make using s_or_saveexec_b64 for entering WWM possible.
• Added tests for WWM across multiple basic blocks and correct SCC handling. For the latter, I couldn't figure out how to make an IR test that exercised that path, so I made a MIR test for it.

Mask out the flag first in markInstruction() to simplify the code a little and
avoid processing instructions unnecessarily once WWM is enabled.

Minor comment style fixes, simplify lowerCopyInstrs by using setDesc()

• Check for correct S_NOP argument and use VI-NEXT
• Only check for VALU EXEC write as per the docs, and update test

Abadon in favor of D35524. While I based that change off of this one, things have changed so much that it's probably better to abandon this and do the review there.

Remove wqm_ctrl in favor of llvm.amdgcn.wqm and llvm.amdgcn.wqm
intrinsics, and remove tests for them.

Remove wqm_mode in favor of llvm.amdgcn.wqm and llvm.amdgcn.wwm intrinsics.

rebase on latest WWM implementation, tweak semantics and implementation
to force WQM whenever WQM is used.

Avoid illegal SGPR<->VGPR copies by treating WQM more like COPY, add test that
exposes the problem.

Ping.

I just posted D35167, which seems closer to how we want to deal with WQM and WWM in the long term, and should provide the same functionality as this intrinsic.

Actually disable WWM on exit of a block.

After some more testing, it seems like my hypothesis about the issue was wrong. I think the real problem was that WWM wasn't being disabled in the face of control flow like I intended it to, and then stuff was getting messed up because inactive lanes were enabled when the code assumed they weren't. At least, that's my theory for now. I'm going to update D34717 to fix that.

In D34847#796305, @michel.daenzer wrote:

This isn't necessary. Exports can't enable pixels that weren't enabled in the first place.

Add a test for the problem as well as a missing test for DPP-after-VGPR-write.

In D34716#794564, @cwabbott wrote:

One thing that strikes me is that while your definition is sufficient for WWM, it isn't for WQM -- for derivatives, GL says that we actually do have to care about the values of things in helper invocations. The program has to behave as if it's always in WQM, except for loads and stores, so just assuming that helper lanes are undefined/poison isn't valid as long as loads from memory aren't involved. I think we can just strengthen the definition of llvm.amdgcn.wqm a little, to say that helper lanes must have the correct value as if everything was computed in WQM.

In D34716#794465, @nhaehnle wrote:

So, one thing that's not clear to me with is the semantics of how the update.dpp intrinsic is supposed to enable WQM or WWM. In your sequence of instructions, if you just put a WQM/WWM flag on the update.dpp intrinsic, how does LLVM know whether the regular ALU intrinsics in between should run in WQM/WWM or not?

Tim had an interesting proposal for that, which involved a pair of intrinsics:

llvm.amdgcn.helpervalue(src, helpervalue) --> returns src for active lanes and helpervalue for other lanes

In D34716#793614, @tpr wrote:

Hi Connor. We have also been thinking about issues around dpp, wqm and wwm inside AMD. Something we may want to see is a way in machine instructions to express that a dpp operand can be combined into an alu op, and then the write gating (bound_ctrl=1, row and bank masks) affects the result of the alu op, not the result of the dpp move. However I guess that is a more extensive change to the definition of a whole class of instructions. I'd be interested to hear your thoughts.

Being able to combine dpp move, alu op and write gating also affects how to express it at the IR intrinsic level. It seems to me that the write gating needs to be a separate intrinsic, with instruction selection spotting that it can all be combined into a single instruction.