[PATCH] D96469: [AMDGPU] Use single cache policy operand

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In D96469#2614150 <https://reviews.llvm.org/D96469#2614150>, @dp wrote:

> MC part overall looks good with minor reservations:
>
> - The change breaks compatibility with existing code. The following code is no longer accepted:
>
>   buffer_load_dword v5, off, s[8:11], s3 lds dlc   // accepted if lds and dlc are swapped

Yes, although I doubt it was ever used. That was not a best idea to break cache policy blocks in the first place.

> Is this an issue? (Fortunately I found no other failures.)

I hope not.

> - With this change assembler will accept duplicate cache policy modifiers. This can be easily amended in some cases, others are more problematic:
>
>   global_atomic_add v[3:4], v5, off slc slc              # easy to detect
>   global_atomic_add v[3:4], v5, off slc noglc noglc      # not so easy to detect
>
> - Some combinations of modifiers will be accepted though they look weird:
>
>   global_atomic_add v[3:4], v5, off slc glc noglc

That is possible to fix I suppose. Initialize CPolSeen = 0 variable at the beginning of the instruction parsing, set the bits for any seen modifier and complain for duplicates.

> - There is a case when assembler incorrectly detects an offending token:
>
>   global_atomic_add v[3:4], v5, off slc noglc glc 

Should also be covered by CPolSeen I suppose.

> - I'd have preferred to enforce cache policy modifiers go in one block (i.e. do not allow them mix with other modifiers). Unfortunately this would result in other incompatibilities with legacy code if implemented.

In fact I would prefer to allow all modifiers in any order and mix, but that's not so easy unfortunately. We knew it from the beginning though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96469/new/

https://reviews.llvm.org/D96469