[AMDGPU] Extend the SI Load/Store optimizer

[piotr]

Summary

Extend the SI Load/Store optimizer to merge MIMG load instructions. Handle
different flavours of image_load and image_sample instructions.

When the instructions of the same subclass differ only in dmask, merge
them and update dmask accordingly.

[arsenm]

I still think we should be handling these on the IR level

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

459–460 ↗

(On Diff #210524)

Can this actually, legitimately fail? I would expect this to be an assert

498 ↗

(On Diff #210524)

C++ style comment

1114–1115 ↗

(On Diff #210524)

Weird to put the assert string first

1120–1125 ↗

(On Diff #210524)

static const

test/CodeGen/AMDGPU/merge-image-load.mir

4 ↗

(On Diff #210524)

Can you include the merged MMO in the check lines

[piotr]

V2: Addressed review comments.

[nhaehnle]

In D64911#1591502, @arsenm wrote:

I still think we should be handling these on the IR level

Matt, where do you think would be the right place to do this at the IR level? Presumably you have an existing pass in mind?

[arsenm]

In D64911#1593515, @nhaehnle wrote:

In D64911#1591502, @arsenm wrote:

I still think we should be handling these on the IR level

Matt, where do you think would be the right place to do this at the IR level? Presumably you have an existing pass in mind?

Either a new pass, or teach the LoadStoreVectorizer about intrinsics. Things are generally easier in the IR, and I don't think this needs any information only available after selection

[piotr]

I think a generic pass would not be suited for our image instructions, due to the dmask special treatment. I had decided to extend the si-load-store-opt pass, because similar transformations were already handled there.

[piotr]

Ping

[piotr]

Rebased and added missing 'const' in SILoadStoreOptimizer::dmasksCanBeCombined.

[piotr]

A recent patch in SILoadOptimizer (D65496) conflicted with this patch. I have rebased it to the latest master, but for clarity I will split the review into two parts: a separate review for the NFC refactoring (D68384) and the current review only for merging MIMG instruction. I will update the current review once D68384 has been merged.

[piotr]

Rebased after D68384 has been merged.

[nhaehnle]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

322–325 ↗

(On Diff #223187)

This should probably check mayStore instead of mayLoad: we want to exclude both stores and atomics.

You could also move the check for TFE and LWE to here.

1051 ↗

(On Diff #223187)

Please use an if-statement.

[piotr]

Rebased and addressed review comments.

[piotr]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

322–325 ↗

(On Diff #223187)

Good point about atomics, I added the condition to bail out on mayStore()). I am keeping !mayLoad() to avoid merging IMAGE_GET_RESINFO.

For TFE/LWE I would like to keep the checks where they are, because I dislike extending getInstClass() with Instruction argument, and it would be necessary to query the actual value of TFE/LWE.

[nhaehnle]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

322–325 ↗

(On Diff #223187)

Can IMAGE_GET_RESINFO not be merged? I would kind of expect it can...

Fair point about TFE/LWE.

[piotr]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

322–325 ↗

(On Diff #223187)

Yes, ideally we would like to combine them too, but currently it would not work, because the pass makes assumptions that the instruction to be merged is either a load or store (assertion thrown otherwise) . I will add a TODO in the code to make it clear that IMAGE_GET_RESINFO is not handled.

[piotr]

Added TODO.

[nhaehnle]

LGTM