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.

Rename a function and add a new helper function based on Matt's comments.

Get the output "Chain" of the intrinsic correctly. Thanks.

Update based on Matt's latest comments.

LGTM

Update based on Reviewer's comments:

ping

ping

Update based on Matt's recent comments. Thanks.

Merge with the latest LLVM trunk.

1. Merge the patch with LLVM trunk.
2. Update LIT tests to avoid specific registers.

1. Add definitions for new machine instructions for D16 bit and for whether target support packed/unpacked f16.
2. Add a flag bit to indicate whether an instruction is referencing 16-bit half typed data (D16).
3. Update to the LIT test cases to avoid using specific registers.

Add LIT tests.

In D39912#922228, @b-sumner wrote:

Pardon my ignorance, but why isn't include/llvm/IR/IntrinsicsAMDGCN.td being updated?

Update based on Matt's comments.

1. Define a new feature, HasPackedD16VMem for gfx8.1 and beyond to guard the generation of VMem instructions with D16 bit set.
2. Put buffer_store intrinsics implementation in the same patch as buffer_loads;

Implement getTgtMemIntrinsic for buffer_load/tbuffer_load intrinsics.
Update based on Matt's review comments.