Bug 102032

Summary:	nir_op_imod is incorrectly implemented as LLVM's srem
Product:	Mesa	Reporter:	programmerjake
Component:	Drivers/Vulkan/radeon	Assignee:	mesa-dev
Status:	RESOLVED NOTABUG	QA Contact:	mesa-dev
Severity:	normal
Priority:	medium	CC:	programmerjake
Version:	git
Hardware:	All
OS:	All
Whiteboard:
i915 platform:		i915 features:

Description programmerjake 2017-08-03 22:08:37 UTC

nir_op_imod is incorrectly translated to LLVM's srem instruction in src/amd/common/ac_nir_to_llvm.c:1567.
If you use the operands -1 and 3, nir_op_imod should return 2, however srem returns -1.

compare with intel's implementation:
https://github.com/mesa3d/mesa/blob/e69e5c7006da80af62c9ef08dec215b3b4b30946/src/intel/compiler/brw_vec4_nir.cpp#L1352

C++ algorithm:
https://godbolt.org/g/pUeEfi

LLVM srem documentation:
https://llvm.org/docs/LangRef.html#srem-instruction

Comment 1 Bas Nieuwenhuizen 2018-01-25 23:19:36 UTC

I went looking to why there were no good CTS tests for this and found this in the vulkan spec:

For the OpSRem and OpSMod instructions, if either operand is negative the result is undefined.

Note
While the OpSRem and OpSMod instructions are supported by the Vulkan
environment, they require non-negative values and thus do not enable additional
functionality beyond what OpUMod provides.


While I'm open to fixing this, you may want to rethink what you are doing.

Comment 2 programmerjake 2018-02-06 04:55:54 UTC

(In reply to Bas Nieuwenhuizen from comment #1)
> I went looking to why there were no good CTS tests for this and found this
> in the vulkan spec:
> 
> For the OpSRem and OpSMod instructions, if either operand is negative the
> result is undefined.

I think this bug should be fixed to support OpenCL. I have not found any references in the OpenCL specs to results of the remainder operator, so I'm guessing it uses the definition eventually derived from C99 section 6.5.5.6 which defines the results for negative operands.

Comment 3 Bas Nieuwenhuizen 2018-02-08 21:05:27 UTC

radv is not a CL driver though, so you can't depend on this working on any unextended vulkan driver.

Comment 4 programmerjake 2018-02-08 23:09:25 UTC

(In reply to Bas Nieuwenhuizen from comment #3)
> radv is not a CL driver though, so you can't depend on this working on any
> unextended vulkan driver.

NIR is not specific to Vulkan though.

Comment 5 Bas Nieuwenhuizen 2018-02-08 23:16:43 UTC

yes, but GL_ARB_spirv has the same spec language and there currently is no CL driver based on the AMD NIR->LLVM code. So none of the currently supported frontends need it.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.