In D73342#1854172, @sheredom wrote:

@hfinkel ping?

@hfinkel ping?

Use combineMetadataForCSE to combine the metadata.

In D73342#1839002, @hfinkel wrote:

In D73342#1838953, @nikic wrote:

Can you explain in more detail why the current behavior is incorrect? Why is using the aliasing information in program order wrong? If the first load is noalias, then we should be able to deduplicate to that noalias load. If the second load is noalias, then generally we can't assume the first load is also noalias. Unless there is a must-exec relation between them, as @nlopes mentioned.

We have two issues here;

1. For any pass, if it changes any instruction with metadata that it does not understand, it should drop that metadata. That's the contract that we need to maintain.
2. For this AA metadata in particular, we need to intersect the metadata, and this is true to preserve semantics for almost all other known metadata (AA, profiling, fp-precision, debug) although likely for it is not correct to keep only one, or the other, because of potential dynamic dependencies on the results.

For AA, as an example, we can have something like this:

int * restrict r = a;
int x = should_alias ? *a : *r;

(essentially the same is true using casts as TBAA).

In any case, we should be calling the function llvm::combineMetadataForCSE here. Please update the patch to do that.

In D71348#1782253, @hfinkel wrote:

In D71348#1779547, @sheredom wrote:

@nhaehnle just added you so that you are aware of this change - it in theory could minorly change codegen on AMDGPU but I think you don't actually return any more precise noaliasing state so it should (tm) be fine. But thought it was worth you oking this either way!

Why would it change the AMDGPU backend? It looks like the AMDGPU backend does provide additional pointsToConstantMemory information via its AA implementation.

@nhaehnle just added you so that you are aware of this change - it in theory could minorly change codegen on AMDGPU but I think you don't actually return any more precise noaliasing state so it should (tm) be fine. But thought it was worth you oking this either way!

In D62198#1510400, @nhaehnle wrote:

I'm personally not confident enough in this change to enable it unconditionally immediately. I think this should be guarded with an option.

Guarded the change by an option to allow users of the structurizer to turn this functionality on in their own time.

In D60600#1485496, @reames wrote:

In D60600#1475212, @sheredom wrote:

In D60600#1470778, @reames wrote:

I think you might be overcomplicating this. I'd suggest the following:

1. scan all index types for a gep
2. run current code, but if any operand was gep, skip the recursive call on the operand

It's slightly less powerful, but correct, and easy. And frankly, optimize geps of struct types is probably rare enough we don't care.

p.s. Agreed on the location of test file, please add to existing tests.

I don't really understand what your suggestion is here sorry - let me try and rephrase the suggestion and see if we agree?

I submitted a version along what I proposed as https://reviews.llvm.org/rL359633. Please confirm that fixes your crash. If you want to work on a better version of the fix, I'm happy to review, but I wanted to make sure the crash was resolved.

Added the extra test case that @spatel found that my change also fixes.

@spatel yup its the same bug, and my fix fixes it to. I'll add it to my test changes.

@reames ping?

Fix some review comments by @spatel

In D60600#1470778, @reames wrote:

I think you might be overcomplicating this. I'd suggest the following:

1. scan all index types for a gep
2. run current code, but if any operand was gep, skip the recursive call on the operand

It's slightly less powerful, but correct, and easy. And frankly, optimize geps of struct types is probably rare enough we don't care.

p.s. Agreed on the location of test file, please add to existing tests.

Found a fun little bug whereby the phys vgprs were being coalesced onto previous instructions, and then shouldClusterMemOps was assuming only virt regs. Added a workaround for that.

Update for two reasons:

Fix review comments.

Added the max3 cases too.

Changed it to a whitelist of types rather than a blacklist (much better idea).

Fixed review comments by Matt and added the extra test cases that were a great idea!

Reduce the number of DPP calls in the test for cleanliness, and reintroduce the convergent on WWM. The CFG test contains the bug that was exposed by the lack of convergent on WWM, LLVM will sink the WWM statement out of the branch which totally messes up all calculations.

Remove the explicit pass name.

Addressed all review comments.

Add a test case that triggers the target transform info code path.

LGTM.

LGTM.

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.