Onednn: ref_convolution get_bias issue

[kruus]

[EDIT: Wrong] In ref_convolution.cpp, the supported type mappings seems to be
wrongly casting s32 pointers to 'const int *'.
I would suggest using prec_traits, which supposedly have the correct types
(in this case s32 --> int32_t, which may or may not be 'int')
[_This was not my issue, although pedantically it still might be nicer to use prec_traits_]

... [EDIT: Right] and then I notice my compiler warns about the type conversion to acc_data_t
so I have a final cast to the correct output type...
[EDIT: _So just the static_cast to acc_data_t eliminates my warnings_. The template instantiations
compile many combinations of bias_desc.data_type and acc_data_type. sxc++ is warning that
"conversion may change the value" a lot. For example, s32 may not be exactly convertible to float,
and float values are not "precisely convertible" to any of the integer types. Yes this is somewhat paranoid.]

convolution.cpp near line 82

      auto get_bias = [=](size_t off) -> acc_data_t {
        switch (conf_.cdesc()->bias_desc.data_type) {
#define SUPPORTED_CASE( DTYPE ) case DTYPE: return static_cast<acc_data_t> \
            (*((const impl::prec_traits<DTYPE>::type *)bias + off));
        SUPPORTED_CASE(data_type::s8)
        SUPPORTED_CASE(data_type::u8)
        SUPPORTED_CASE(data_type::s32)
        SUPPORTED_CASE(data_type::f32)
#undef SUPPORTED_CASE
        default: assert(!"unimplemented");
        }
        return 0;
    };

[emfomenk]

Hi @kruus,

Good catch.
In general, you are right -- we cannot assume int == int32_t.
Though for all target HW (which is basically x86/x86_64 Lin/Win/Mac) the assumption is correct.
So we might want to get rid of int32_t type and just use int instead, and do not mess one with another.
That will also fix the compiler warning you got.

What compiler do you use?

[emfomenk]

Hmm, I looked at your fork of MKL-DNN.
Could you please say a few words regarding the purposes of the project?
It would be really interesting.

Would you expect on some the platforms you need int != int32?

[kruus]

I think that MKL-DNN is a really well thought design for any low-level deep-learning library.
I have proposed that NEC use the MKL-DNN API for for future SX-ACE style HPC systems,
and NEC was receptive to this proposal.
MKL-DNN design is also really great in that it was really easy to add a new subdir with a
new engine and my own particular set of allowed primitive impls (ex. for SX CPU/OS, TBD)

My compiler is the sxcc cross-compiler, which is pretty strict about warnings for
any precision loss (including signed/unsigned conversions and double->float
and int->float issues). It is a fairly advanced vectorizing compiler.

The SX "HPC" CPU has very long simd registers, and sxcc has supported for a long time
counterparts to many of the newer AVX2/AVX-512 instructions. The compiler docs show
how to generate different assembly code variations by different C loop idioms. My previous
work suggests SX may do fairly well by bolstering the ref impls with C++ impls tailored to
generate fast SX assembly code.
SX gemm impl I found handles tall-skinny corner cases better than than MKL2017.
(im2col convolution is sometimes slower on Intel than an alternate gemm impl using a *hwc layout,
but im2col gemm convolution is almost always the fastest impl for SX, for example).

If there is other interest in supporting other non-JIT chipsets one general change would
be a single flag to completely disable all JIT code. This would make
an initial (really slow) version that runs ref/mkl/cblas impls on "any" system even easier
to create. I.e. just copy the cpu/ subdir, compile with "no-jit" flag, and then add custom impls
to your cpu_engine.
It is already pretty easy though, thanks to your excellent design work!

[kruus]

int32_t: Oh, right you are. int happens to be int32. So maybe I was just seeing
ultra-paranoid sxcc "int vs size_t" warnings. For sxcc size_t is configurable as 32- or 64-bit.
I guess x86 size_t has similar issues (ex. gcc -m32 switch) but gcc may be less paranoid.

The real question is should s32 always try to be 32-bit? I would guess yes.
In this case int32_t is a good choice (or maybe even int_least32_t).
I would find it strange if I ask for "s32" data type and have 64-bit values (2x the memory)!
Also, having the s32 code look the same as u8/s8/s16 cases seems intuitive.

In any case, the choice should consolidated into a single location, prec_traits.
(This was the only case I found where local type differed from the prec_traits specification)

I will look closer at what was happening with sxcc to generate the warning
next week -- pleasant holidays :)

[rsdubtso]

So we might want to get rid of int32_t type and just use int instead, and do not mess one with another.
That will also fix the compiler warning you got.

Maybe I'm reading this wrong, but I don't think that would be the right way to do it. It seems natural to have a mapping of s32 -> int32_t, u32 -> uint32_t. I think @kruus is saying the same thing: s32 is a promise for a 32-bit signed integer type, so it would be weird for the library to use anything else instead.

PS. Re: design. We are not happy with the current state of the things, actually :) From the top of my head: the library has hard time supporting copy-less tensor splits, copying data between engines is also quite awkward. If you find other gaps, please let @emfomenk and me know.

[emfomenk]

oK, my bad, agree.
You both convince me :)

Will fix that soon.