Opened 3 years ago

Last modified 7 months ago

#564 reopened defect

improve support for tgamma and other missing function

Reported by: smk78 Owned by:
Priority: minor Milestone: SasView Next Release +1
Component: SasView Keywords:
Cc: Work Package: SasModels Infrastructure

Description

Executing make clean html from Windows CMD or Git Bash shell:

python genmodel.py ../sasmodels/models/fractal.py model/fractal.rst
Traceback (most recent call last):

File "genmodel.py", line 103, in <module>

plot_1d(model, opts, ax1d)

File "genmodel.py", line 47, in plot_1d

Iq1D = calculator()

File "e:\sasviewdevelopment\sasmodels\sasmodels\direct_model.py", line 214, in call

return self._calc_theory(pars, cutoff=self.cutoff)

File "e:\sasviewdevelopment\sasmodels\sasmodels\direct_model.py", line 174, in _calc_theory

self._kernel = self._model.make_kernel(self._kernel_inputs) # pylint: disable=attribute-dedata_type

File "e:\sasviewdevelopment\sasmodels\sasmodels\kernelcl.py", line 320, in make_kernel

self.fast)

File "e:\sasviewdevelopment\sasmodels\sasmodels\kernelcl.py", line 224, in compile_program

program = compile_model(self.get_context(dtype), source, dtype, fast)

File "e:\sasviewdevelopment\sasmodels\sasmodels\kernelcl.py", line 150, in compile_model

program = cl.Program(context, source).build(options=options)

File "c:\Anaconda\lib\site-packages\pyopencl\init.py", line 213, in build

options=options, source=self._source)

File "c:\Anaconda\lib\site-packages\pyopencl\init.py", line 253, in _build_and_catch_errors

raise err

pyopencl.RuntimeError?: clBuildProgram failed: build program failure -

Build on <pyopencl.Device 'Oland ' on 'AMD Accelerated Parallel Processing' at 0xa9f5210>:

"C:/Users/smk78/AppData/Local/Temp\OCL7C94.tmp.cl", line 210: error:

storage-class specifiers extern/static/register not allowed for
variables

static float SQTPI = 2.50662827463100050242E0f;

"C:/Users/smk78/AppData/Local/Temp\OCL7C94.tmp.cl", line 232: error:

identifier "SQTPI" is undefined

y = SQTPI * y * w;


"C:/Users/smk78/AppData/Local/Temp\OCL7C94.tmp.cl", line 238: error:

storage-class specifiers extern/static/register not allowed for
variables

static float LOGPI = 1.14472988584940017414f;

"C:/Users/smk78/AppData/Local/Temp\OCL7C94.tmp.cl", line 291: warning: goto

statement may cause irreducible control flow

goto small;

"C:/Users/smk78/AppData/Local/Temp\OCL7C94.tmp.cl", line 299: warning: goto

statement may cause irreducible control flow

goto small;

3 errors detected in the compilation of "C:/Users/smk78/AppData/Local/Temp\OCL7C94.tmp.cl".

Frontend phase failed compilation.

(options: -I c:\anaconda\lib\site-packages\pyopencl\cl)
(source saved as c:\users\smk78\appdata\local\temp\tmpbnhiza.cl)
make: * [model/fractal.rst] Error 1

smk78@NDW1000 /e/sasviewdevelopment/sasmodels/doc (master)
$

Change History (6)

comment:1 Changed 3 years ago by smk78

Resolved by PK's commit e07b470c1b10dba7e48f01177a6f28fec78e3259.

comment:2 Changed 3 years ago by smk78

  • Resolution set to fixed
  • Status changed from new to closed

comment:3 Changed 3 years ago by pkienzle

  • Resolution fixed deleted
  • Status changed from closed to reopened

Although the docs are building there is still a warning from the compiler that code including goto is being run on the GPU.

The problem stems from a patch to extend MSVC with the tgamma function which is showing up in OpenCL even though it is not needed there. The code includes some goto statements which are not supported by GPUs.

The test for whether the code is included uses #ifdef tgamma. At a guess, some platforms define tgamma as a #define to some internal implementation, but apparently not all opencl headers (Radeon Nano on Linux for example). We can instead put a #define NEED_TGAMMA in the compilers that needed it (tinycc and msvc on windows), then test that flag to see if we really need to provide an implementation of tgamma. Similarly for expm1. This will also make the headers a little cleaner.

The sas_gamma = tgamma(x+1)/x definition is still needed, even with the replacement tgamma. IIRC, this form is more numerically stable for x<1. Ideally, I would like this to become the definition of tgamma, and not use the sas_gamma name in the code. Maybe the following in the header?

   #if defined(NEED_TGAMMA)
   … cephes tgamma …
   #endif  // NEED_TGAMMA
   // sas_gamma uses tgamma from the environment, or the cephes version defined above
   static double sas_gamma(double x) { return tgamma(x+1.)/x; }
   // protect against the tgamma definition being a macro expansion
   #if defined(tgamma)
   #undef tgamma
   #endif
   #define tgamma(_v) sas_gamma(_v)

That way those who are writing models can just use tgamma but still have it be accurate.

Note that tgamma might be defined as a macro in order to support type generic float tgamma(float) etc. This isn’t actually an issue for us because I’m substituting the word double for float for single precision, so the tgamma picked up from the environment will be the tgamma appropriate for the type.

I prefer to have tgamma/expm1 in the kernel header section rather than the library because it makes it easier to steal the header code and use it in other projects, but I’m okay with treating it more like a library as well, along with sinc, square and cube.

Some compilers are complaining about the unused static defs for sinc, square and cube. We could play at being a linker as well, and only include them if the source code uses those names. Similarly for tgamma and expm1 if we need to include our own versions.

comment:4 Changed 3 years ago by pkienzle

  • Milestone changed from SasView 4.0.0 to SasView Next Release +1
  • Priority changed from blocker to minor
  • Summary changed from sasmodels doc build is failing on W7 to improve support for tgamma and other missing function

The blocker aspect has been removed: opencl again uses the built-in tgamma function.

Still need to provide single precision versions for completeness in case users want single precision in DLLs.

Still would be nice to make a fake linker so that only the needed functions are included.

Still would be nice to make tgamma accurate rather than the user knowing they need to call sas_gamma instead.

comment:5 Changed 13 months ago by smk78

  • Work Package changed from SasView Documentation to SasModels Redesign

This is no longer a documentation task. Reassigning it to SasModels Redesign.

Last edited 13 months ago by smk78 (previous) (diff)

comment:6 Changed 7 months ago by butler

  • Work Package changed from SasModels Redesign to SasModels Infrastructure
Note: See TracTickets for help on using tickets.