I am working on a regression caused by this patch. It is a memory access fault actually.
However, if we put back the original consition, my test would pass.

Add comment to the test

update the test with update_test_checks.py

Fix a typo and add a test.

Remove the unused Target Instruction Info argument in a couple functions.

Use MachineInstr::mayAlias to replace areMemAccessesTriviallyDisjoint in LoadStoreOptimizer pass.

Update based on reviewer's suggestions:

1. -check-prefix=GCN
2. addrspace(0) is not needed.

In D56454#1350348, @arsenm wrote:

This problem isn't limited to private address space. This should have tests for every address space, and with cases using unrelated bases

In D56454#1350293, @rampitec wrote:

Can you add a test where low half does not produce a chain? An arithmetic operation and an undef.

Update based on the comments from the reviewers:

update the test based on the following suggestion:

There should be no unnamed variables left.

Cheap check first.

In D55241#1323977, @arsenm wrote:

No, it's not committed. Variable + constant is a common case in general.

It would probably be better to do this fold in the DAG for now though

Fix typos.

In D55241#1317703, @arsenm wrote:

D30466 is the primitive computeKnownBits

In D55241#1317688, @arsenm wrote:

In D55241#1317631, @cfang wrote:

In D55241#1317630, @arsenm wrote:

We should try to use some known bits information to keep this. I have a patch to add a machine version, but there might be a better way

Would you please explain how would your knownbit approach resolve the negative index issue while keep the optimization for gfx9+?
Or just post your patch. Thanks.

If you know the base index isn't negative, you don't need to disable this

In D55241#1317630, @arsenm wrote:

We should try to use some known bits information to keep this. I have a patch to add a machine version, but there might be a better way

Add test for non-uniform case.

Thanks Justin for your comment. I added Daniel as a reviewer.

Updated the test based on the comments

Right. Reverse Post Order (RPO) should be the fundamental order. But apparently there are some cases that pure RPO does not work.
That's the reason loop depth was introduced to guard the ordering of the nodes.

We need this fix to unblock some important tasks. So if there is no additional comments, we need the permission
to integrate the patch. Thanks.

ping

Any additional comments/suggestions? Thanks!

Update based on Matt's comments.

Correct indentation error and typo.

In D46340#1086444, @nhaehnle wrote:

So this does not cover all possible cases of "irreducible" infinite loops, e.g. consider the case with basic blocks A and B where both end with a conditional branch that can go to either A or B. I.e., there are no unconditional branches in the loop.

A more robust approach would be to also transform conditional branches like this:

br i1 %cc, label %A, label %B

becomes

  br i1 true, label %DummyReturnBB, label %local_dummy
local_dummy:
  br i1 %cc, label %A, label %b

Handle the case that the infinite loop is controlled by a conditional branch. In such case, the two edges of the branch
are both backedges.

Update the test:

Update the test:

1. Remove the -amdgiz from the triple since it is no longer necessary;
2. Add "-data-layout=A5" to explicitly specify the data layout for address space 5 for alloca
   • and to remove the "target:" line for the same data layout purpose.

LGTM

What do you think of the test?
What should we do if we don't add the following line?
target datalayout = "A5"

What do you think of the test? Thanks;

LGTM,

Combine !isAtomic and !isVolatile checks as isSimple

Fix addrspacecast in the test since generic address space is 0 (default) now. Thanks, Sam!

Add a test.

Add reviewers and ping.

ping

Update LIT tests before landing since the patch was developed a long time back.

full context diff

Ping

LGTM

In D42548#993741, @arsenm wrote:

Needs test

Update tests to expose the LLVM-ERROR withput the patch.

ping! Thanks.

In D42596#990403, @dp wrote:

It would be nice to have a few decoding tests as there is currently zero _d16 coverage for disassembler.

BTW, there is no difference in data size for affected MIMG instructions. Currently data size in dwords = count_population(dmask) + tfe. How does d16 affect this number?

I was unable to find anything helpful in docs and SP3 output is not affected by d16 for gfx9. Are data in VGPRs really packed for MIMG? :-)

For MIMG, to determine the data size for d16, we need to know whether the target has the feature UnpackedD16VMem (gfx8.0),

1. if that feature is set, the data size is the same as without D16 bit set;
2. if that feature is not set, then the data size is "half" of the size because we can packed two f16 into one register.

Add Disassembler tests based on Reviewers' suggestion. Thanks.

In D42596#989475, @b-sumner wrote:

Can this be tested?

Patch committed to trunk:
git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@322903

Patched updated! Request for reviewer's check. Thanks.
<still I could not receive message of the diff update>

Update based on Matt's suggestion: Factor out the common defs.

Don't know why I didn't received a message after I updated the patch. So ping here with the updating message:

1. sync with the buffer d16 support patch to reuse some of the functionalities;
2. Remove unnecessary functions that print "d16" in the AsmPrinter;
3. Update based on Matt's other comments.

Redesign the function to handle Vdata to store so that is will be called only when vdata is of type d16.