Bug 102032 - nir_op_imod is incorrectly implemented as LLVM's srem
Summary: nir_op_imod is incorrectly implemented as LLVM's srem
Status: RESOLVED NOTABUG
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Vulkan/radeon (show other bugs)
Version: git
Hardware: All All
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-03 22:08 UTC by programmerjake
Modified: 2018-02-08 23:16 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

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.