Lower krnl.call to llvm #1408

chentong319 · 2022-05-05T20:13:29Z

Modified the krnl.call definition and builder.
The new krnl.call supports that the returned value is a tensor, which is passed as a parameter to krnl.call. The krnl.call itself has no output. The read/write effect for parameters are set.
Lower krnl.call to llvm
Parameters and attributes of krnl.call Op are turned into parameters for LLVM call op. The function type is determined by the types of parameters. In general, parameters with the tensor type or string type are passed by address, and those with scalar types are passed by value.
Test with 6 new test cases that use the krnl.call.
Since there is no lit test for krnl to llvm, I tested this PR with backend test. Since ResizeOp is quite complicated for deferent configuration, I manually translated the python implementation of ResizeOp in onnx package into c. 6 test cases are added to backend test.
Some document for krnl.region

This PR can be improved in several places. Since it is already quite large, I will leave the improvement for future PRs:

Handle optional input tensor with NoneType in a more generic way. Hopefully, null pointer can be used and leave the runtime library to handle it, reducing the number of runtime functions needed.
Propagate attributes with default value. Such attribute may not appear in the Op and is lost when the op is translated to krnl.call. Also all the attributes are blindly copied. If an attribute, not from ONNX Op specification, is added, krnl.call will have an extra parameter for that attribute.
The Resize runtime is very inefficient.
Only some configuration of ResizeOp is supported by runtime library currently.

Signed-off-by: chentong319 <chentong@us.ibm.com>

chentong319 · 2022-05-06T14:24:12Z

The failure on Windows is caused by c compiler for runtime library. Dynamic arrays are used in the code, which is not accepted by the windows. The code looks like the following:

void foo(int n) {
float a[n];
}

Related to this issue, array of dynamic array is not accepted by g++. It seems to me that I have to change the code to avoid that feature.

void f(int n, float a[][n]){
    a[k][j] = 0;
}
void g(int n, int m) {
    float b[m][n];
    f(n, b);
}

Signed-off-by: chentong319 <chentong@us.ibm.com>

etiotto · 2022-05-31T19:08:38Z

src/Runtime/OMResize.cpp

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+//===--------- OMResize.cpp - OMResize C++ Implementation ---------===//


Fix line length

etiotto · 2022-05-31T19:08:52Z

src/Runtime/OMResize.cpp

+
+//===--------- OMResize.cpp - OMResize C++ Implementation ---------===//
+//
+// Copyright 2019-2021 The IBM Research Authors.


Copyright would be 2022 for new files.

etiotto · 2022-05-31T19:16:12Z

src/Runtime/OMResize.inc

+ **/
+
+static void linear_coeffs(float ratio, float *coeffs_buffer, int mode) {
+  coeffs_buffer[0] = 1 - ratio;


I'm just looking to this code without context and wondering how can we guarantee that coeffs_buffer has at least 2 elements. Also coeffs_buffer could be null. You could change the function signature to:

static void linear_coeffs(float ratio, float coeffs_buffer[2], int mode) {

Also mode is not used, can you remove it pls

The implementation is translated from python code. Some types and parameters could be optimized as you suggested. I may keep some interface as its python counter part before the code is thoroughly tested.

Those *_coeffs functions are passed by function pointers. The coeffs_buffer may have 2 or 3 elements. I changed code pointer for array declaration. I feel the benefit is to clearly show (to user) that at least how many elements this pointer should have. But the compiler will still treat that parameter as just pointer.
To have the same signature for function pointer, parameter mode can not be deleted even it is not used in one function.

etiotto · 2022-05-31T19:17:21Z

src/Runtime/OMResize.inc

+  coeffs_buffer[1] = ratio;
+}
+
+static void nearest_coeffs(float ratio, float *coeffs_buffer, int mode) {


float *coeffs_buffer --> float coeffs_buffer[2]

etiotto · 2022-05-31T19:24:09Z

src/Runtime/OMResize.inc

+  /* integer ratio is handled outside */
+  switch (mode) {
+  case 0: // round_prefer_float
+    coeffs_buffer[0] = ratio <= 0.5;


coeffs_buffer is an array of float, but the values stored into it are actually of type _Bool. Suffest to change its data type to _Bool for C or bool for C++.

The original code takes the conversion of bool to float. I change the code to make it explicit: coeffs_buffer[0] = ration <= 0.5 ? 1 : 0;, for all the assignments.

etiotto · 2022-05-31T19:49:30Z

src/Conversion/ONNXToKrnl/Tensor/Resize.cpp

@@ -129,12 +129,27 @@ struct ONNXResizeOpLowering : public ConversionPattern {

    // Call external function when the mode is not "nearest"
    // Create KrnlCallOp and replace the du chain
+    // One of inputs, scales() and size(), has to be None.


One of the inputs, 'scales' or 'size', has to be 'None'.

Yes, one of them has to be None. In general, the semantics of None is interpreted at the ONNX Op level. We could use tensor<0xT> for None and lower it to memref. So that we can keep None in Krnl and llvm after ONNX Op. We may have another PR for that functionality.

etiotto · 2022-05-31T19:51:49Z

src/Runtime/OMResize.inc

+      /*exclude */ 0);
+}
+
+void Resize_Size(


[nit]: I would name this resize_size for consistency. Similar for resize_scales.

Resize comes from the ONNX Op name and Size or Scales comes from the attribute name. I can change function name for these two. But I am thinking of constructing the function name automatically. So can we just leave them?

etiotto · 2022-05-31T19:52:25Z