search

KhronosGroup / SPIRV-LLVM-Translator

A tool and a library for bi-directional translation between SPIR-V and LLVM IR
Other
468 stars 209 forks source link

[SPIR-V 1.6] Removed OpLessOrGreater #2527

Closed vmaksimo closed 2 months ago

vmaksimo commented 2 months ago

LGTM. But, do we want to add a quick test here? Also, not a blocker for this PR: Having 'asserts' means that these cases will not be caught in a release build. Is that agreeable?

@asudarsa Unfortunately, we don't have a good mechanism to check assertions in lit testing, so a quick test can only verify that the test fails but without any details. Although I verified locally that we do fail with the added error in case of invalid SPIR-V.

About assertions, I believe we agreed in previous PRs that for now we don't have another way to report error from validate() method.

LU-JOHN commented 2 months ago

Should we also add assertions for other deprecated instructions: OpKill, OpAtomicCompareExchangeWeak, OpGroupMemberDecorate, OpGroupDecorate, and OpDecorationGroup?

vmaksimo commented 2 months ago

Should we also add assertions for other deprecated instructions: OpKill, OpAtomicCompareExchangeWeak, OpGroupMemberDecorate, OpGroupDecorate, and OpDecorationGroup?

Let's do it in separate PRs. @LU-JOHN If you don't mind, could you please create and assign such issue to me?