In D57737#1392667, @tpr wrote:

But I still don't understand it:

1. Why do you want an exclusive scan? Surely what you're trying to do is just "sum" up all lanes into lane 63, which is an inclusive scan.

Fix comment that should now say exclusive scan instead of inclusive scan.

Make the final readlane be in the WWM section too as per the review comments.

Updated to bring in an additional fix to remove read_register exec and replace it with a ballot.

Fixed review comments.

Fix review comments.

Review comments.

Also tag s.getpc as scalar, and add tests in the Analysis folder like suggested.

@arsenm agreed - not quite sure how to do tests for this? I started to look at how we test alias analysis, and they have a cl::opt to force print the debug info for that pass, and that is how our amdgpu-alias-analysis.ll tests the aliasing rules. How exactly to test that divergence analysis did something here?

@cwabbott I planned to do a followup once this DPP change had landed to add the missing dpp/codegen patterns to the atomic optimizer - so watch this space!

Fix review comment.

Fix review comments.

@nhaehnle I've removed canReadVGPR as it only had the single callsite - but given @arsenm's comment I did not change getOpRegClass. I also don't feel super comfortable doing the change you suggested to addUsersToMoveToVALUWorklist as I'm worried that there will be non-LLVM-tested paths that I could trip up on with ease. I'd rather ship the commit as is if y'all are ok with it.

Removed canReadVGPR as it had only a single callsite.

Made the l1 flush change happen for MESA3D too (like Nicolai asked for).

Fixed review comments:

We discussed this on an internal AMD meeting Monday 5th November 2018, and came to the conclusion that even though I do want the scalar load combining to be brought upstream, it would be better as a separate change so that we can get broader testing across the users of our AMDGPU backend.

Fix review comments made by @nhaehnle

Review fixes.

Added an additional lit test for the dim variants (including the 2darraymsaa requested by @nhaehnle), and change the naming of the a16.d16 tests to vNf16.

Maybe a dumb question - but why can't we just use the tbuffer load/store instead of these? It already upcasts for you (the zext/sext is built in depending on the nfmt I believe).

Rebased ontop of tip to get the IRBuilder change in https://reviews.llvm.org/D52087, and incorporate the requested changes by @nhaehnle resulting from that.

In D52548#1249085, @spatel wrote:

Thanks - LGTM. As before, I'd prefer to have the baseline tests in place before the patch. Let me know if I should commit on your behalf.

Fix latest review comments.

Move the shuffle widens check into Instructions.h, and call it increasesLength to match the existing changesLength call that was already on ShuffleVectorInst.

Changed the check to not ever push wider shufflevector stuff back onto predecessor instructions as per spatel's suggestion!

Incorporate a related test case that my approach also fixes from https://bugs.llvm.org/show_bug.cgi?id=28911

In D52540#1247810, @uabelho wrote:

dstenb described a possible fix in a comment of PR28911 and we've been using that fix for a long time now for our
out-of-tree target without problems.

Perhaps that fix is a hack and using RPOT is the proper way to deal with this, I've no idea. I just wanted to point out the
possibility.

In D52540#1247681, @uabelho wrote:

Is this the same problem as described in https://bugs.llvm.org/show_bug.cgi?id=28911 ?

Rebased on tip trunk.

• Added a comment explaining each of the 3 variants for each of sdiv/srem/udiv/urem
• Fixed the whitespace issue
• Removed the FMF flags that were a legacy from the original shader this was reduced form

Removed the fdiv/frem as they weren't currently inst simplifying the bad behaviour, and used the utils script to update the test case.

See https://reviews.llvm.org/D52556 for just the test case with the bad output expected.

Just to be clear you want me to commit the tests in a separate commit with the bad output first?

In D51969#1244238, @mareko wrote:

What happens if a shader already does "if (threadID == 0) { do_atomic(); }"? Is the optimization skipped in this case?

Removed the no-args overload as per Nicolai's suggestion.

Added an interaction with the DominatorTree, so that if its present in the PassManager we can preserve + update it.

Update to include tip LLVM changes (DivergenceAnalysis -> LegacyDivergenceAnalysis, AMDGPUAS is become just an enum now).

This is something we (Codeplay) would like to see upstream, it is a much cleaner solution than the metadata workarounds everyone (including us) have been using to fix this.

Thanks!

So we build a bunch of internal libraries in a mix of OpenCL and C++, and then link them all together to create SPIR libraries that can be fed to calls to clLinkProgram and linked against user kernels.

Ran clang-format on the file, and there was a TON of line changes out-with the patch. Extracted the formatted lines from my original patch only, and updated the patch.