ENH: unify PADDLE_ENFORCE for cublas, cudnn, curand

[gangliao]

No description provided.

[reyoung]

PADDLE_ENFORCE means the condition must be true. But CUXXXX_STATUS_SUCCESS is zero, and false. So it seems that these lines should be

PADDLE_ENFORCE(cudaStreamCreate(&stream_) == CUDA_SUCCESS);

[gangliao]

In my opinion, PADDLE_ENFORCE(condition, ...) means condition must execute correctly. No matter the return of condition is 0 or 1, the internal of PADDLE_ENFORCE can figure out how to deal with it.

[QiJune]

Agree with @reyoung. I check on how glog does. What CHECK does is to check the condition is true.
And we can implement CUDNN_ENFORCE/CUBLAS_ENFORCE based on PADDLE_ENFORCE for writing related codes more conveniently

[wangkuiyi]

Given that

PADDLE_ENFORCE(cudaStreamCreate(&stream_))

is shorter than

PADDLE_ENFORCE(cudaStreamCreate(&stream_) == CUDA_SUCCESS);

and it is compatible with the use of the word "enforce" in English, I support that we adopt @gangliao 's proposal -- to make PADDLE_ENFORCE handle the condition.

I agree the English word CHECK should be followed by a boolean value and expects to be true. My point is that ENFORCE should be followed by an action, which is hopefully successful.

[gangliao]

Yeah, @wangkuiyi

PADDLE_ENFORCE(cudaStreamCreate(&stream_) == CUDA_SUCCESS);

will also impede us to print apposite error info for cuda, cudnn, cublas, curand.

[wangkuiyi]

It seems that this snippet

  if (e) {
    std::stringstream ss;
    ss << ::paddle::string::Sprintf(args...);
    ss << ::paddle::string::Sprintf(" at [%s:%s];", __FILE__, __LINE__);
    throw thrust::system_error(e, thrust::cuda_category(), ss.str());
  }

can be rewritten as

  if (e) {
    throw thrust::system_error(e, thrust::cuda_category(), 
                               string::Sprintf(args...) + string::Sprintf(" at "...));
  }

I assume that we can write string::Sprintf other than ::paddle::string::Sprintf.

[wangkuiyi]

How about rephrase this comment:

// Because most enforce conditions would evaluate to true, we can use 
// __builtin_expect to instruct the C++ compiler to generate code that always forces 
// branch prediction of true. This generates faster binary code. For more details, 
// please check https://stackoverflow.com/a/43870188/724872. __builtin_expect
// is since C++11.

[wangkuiyi]

It seems that we don't really need PADDLE_THROW or EnforceNotMet; instead, we can call

throw std::runtime_error(...);

[gangliao]

I totally agree with you, I will remove EnforceNotMet and PADDLE_THROW

[wangkuiyi]

I noticed that paddle::string doesn't call paddle::platform now, so it is safe to make paddle::platform call paddle::string. I think this is the right calling direction. Thanks!

[wangkuiyi]

How about we don't make these throw_on_error variants inline functions, but just normal functions, so we could move their implementations into a .cc file and make this header files concise and clean? It seems that this wouldn't degrade the performance of PADDLE_ENFORECE.

It looks to me that it's reasonable to keep this header file short; currently, the #ifndef PADDLE_ONLY_CPU check spans over too many lines.

[gangliao]

That's a good idea, but enforce.h is always frequently used in almost all source files.
If we separate it to *.h，*.cc, then almost all files have to compile with libenforce.a as follows:

cc_library(each_file SRCS each_file.cc DEPS enforce)

That's quite annoying.

[wangkuiyi]

In addition to a int stat variant, should we also have a bool variant? I noticed applications like

PADDLE_ENFORCE(paddle::platform::is_cpu_place(self.place()),
                        "Only CPU tensor can cast to numpy array");

[gangliao]

I will double check this, because we already use bool condition in our tensor implementation, but it didn't throw a error.

[QiJune]

The pr modifies many files, does it modify automatically or by hand?

[gangliao]

@QiJune Using IDE or Modern editor like VS CODE can easily achieve this.