[clang] [llvm] [SPIRV][RFC] Rework / extend support for memory scopes (PR #106429)

[Vyacheslav Levytskyy via cfe-commits]

VyacheslavLevytskyy wrote:

Thank you @AlexVlx for answering some of my questions. It was exactly my point in the comment that clang is not the only available FE and SPIRV BE should remain agnostic towards vendors, frameworks, etc., so it's nice to be on the same page wrt. vendor-specific choice of terminology (the use of "singlethread" is justified, but "all_svm_devices" is not) and/or the choice of default values (the System scope in the role of a stricter default is a reasonable justification). SPIRV BE needs to accommodate any flavor of SPIR-V Compute with equal hospitality, so, continuing your metaphor, that train still waits on the platform, it's just a matter of will and considerate manner of changes.

Just a couple of comments:

> Perhaps a better solution is to add an option that controls this behaviour / the default, that the BE can consume and act accordingly?

It doesn't seem necessary to get one more command line arg to cover this, it doesn't help more than setting an explicit scope in a (highly hypothetical) customer's code.

> I believe the current string based design was put in place to give full freedom to targets, so relying strictly on integer values for scopes is legacy/less preferred.

I don't agree. Existing implementation gives the same level of freedom to targets, using string values in the same way. However, we don't expect a target to change string names of scopes between getMemScope() calls, so there is no justification to use strings in every call instead of accounting for string names just once. I'd vote to keep the current approach as more performant.

Talking about the best way to move forward, I don't see any contention points. I'd be glad to hear from you at the SPIR-V BE discussion group where it should be possible to talk this out faster/easier, or just here, both will do. Probably topics comprise just (1) the default value, (2) how to spell the word "all_svm_devices" properly to remain agnostic wrt. vendors (would it be "system"? "crossdevice"? anything else?), and (3) no reason behind getSyncScopeNames() asking string scope names on each call to getMemScope() instead of integers as now (i.e., if we indeed need to change the default, it looks a reasonable decision to integrate it with the existing code).

https://github.com/llvm/llvm-project/pull/106429