[cker/train] Introduce Pad op in train #12522

YongseopKim · 2024-01-24T08:16:28Z

This commit introduces Pad op in cker/train.

ONE-DCO-1.0-Signed-off-by: Yongseop Kim [email protected]

This commit introduces Pad op in train. ONE-DCO-1.0-Signed-off-by: Yongseop Kim <[email protected]>

jyoungyun · 2024-01-24T08:39:28Z

compute/cker/include/cker/train/operation/Pad.h

+template <typename T>
+inline void Pad(const int32_t *padding_data, int32_t pad_rank, const Shape &input_shape,
+                const T *input_data, const Shape &output_shape, T *output_data,
+                const T *constant_value_data)
+{
+  // Use nnfw::cker::Pad directly
+  nnfw::cker::Pad<T>(padding_data, pad_rank, input_shape, input_data, output_shape, output_data, constant_value_data);
+}


IMHO, there is no need to re-define this function. We can use nnfw::cker::Pad directly in train backend. It makes unnecessary function call and we have to manage two set of code (Pad in train/operation and Pad in cpu/operation). Also it makes binary size bigger. (Of course, it's a very trivial issue. 😄)

https://github.com/Samsung/ONE/pull/12522/files#r1465679532

YongseopKim · 2024-01-24T23:40:59Z

compute/cker/include/cker/train/operation/Pad.h

+ *             ->    [C,C,C,C]
+ */
+template <typename T>
+inline void Pad(const int32_t *padding_data, int32_t pad_rank, const Shape &input_shape,


My new patch overwrites @jyoungyun 's comment. I write here again instead of @jyoungyun .

#12522 (comment)

IMHO, there is no need to re-define this function. We can use nnfw::cker::Pad directly in train backend. It makes unnecessary function call and we have to manage two set of code (Pad in train/operation and Pad in cpu/operation). Also it makes binary size bigger. (Of course, it's a very trivial issue. 😄)

I agree with @jyoungyun . I'm going to use nnfw::ckwer::Pad in train backend.

YongseopKim · 2024-01-24T23:55:43Z

PTAL @jyoungyun and @Samsung/one_onert

jyoungyun

LGTM

jyoungyun · 2024-01-25T01:04:19Z

compute/cker/include/cker/Types.h

+struct PadParams
+{
+  int32_t data[8];
+  int32_t rank;
+};


👍

It would be good to use this param in cpu backend too.

compute/cker/include/cker/train/operation/Pad.h

YongseopKim · 2024-01-25T06:02:10Z

PTAL

zetwhite · 2024-01-25T06:59:51Z

compute/cker/include/cker/train/operation/Pad.h

+            const auto in_offset = (c + padding_cube) * in_cube_size +
+                                   (d + padding_depth) * in_plain_size +
+                                   (h + padding_top) * in_width + (padding_left);
+            const auto out_offset = (c * out_cube_size) + (d * out_plain_size) + (h * out_width);
+            // copy a row of input data to output data
+            std::memcpy(output_data + out_offset, input_data + in_offset, out_width * sizeof(T));


I'm a bit afraid that I might be wrong.

But this offset calculation looks like NCHW. 🤔

const auto in_offset = (c + padding_cube) * in_cube_size + // n * CHW (d + padding_depth) * in_plain_size + // c * HW (h + padding_top) * in_width // h * W + padding_left // w

I referred this : https://oneapi-src.github.io/oneDNN/dev_guide_understanding_memory_formats.html#nchw

Be not afraid 😄. My code could be wrong. I'll take a look more. Thank you!

IHMO, in cker, the logic here doesn't matter NCHW/NHWC. In the view of Pad kernel, it just does copying. If it might be NCHW, the code var name will be changed.

If I'm wrong, I'll leave a comment.

IHMO, in cker, the logic here doesn't matter NCHW/NHWC.

I agree somehow.

IMHO, layout in onert(ir::Layout::NHWC) just meansnnfw::cker::Shape's element order.
In the tensor memory level, offset-calculation can be different.

BTW mostly looks good.
Let's just clarify this layout things and process it 😄

Okay now I'm sure somewhat.

At the train backend level, tensors' layer assume NHWC.

// backend/train/Config.cc ir::Layout Config::supportLayout(const ir::IOperation &, ir::Layout) { return ir::Layout::NHWC; } // backend/train/BackendContext.cc backend::train::ITensorRegistry *BackendContext::genTrainingTensors() { const ir::train::TrainableGraph &tgraph = *trainable_graph(); auto tensor_builder = _tensor_builder; tgraph.operands().iterate([&](const ir::OperandIndex &ind, const ir::Operand &obj) { if (external_operands().contains(ind)) return; // NOTE Assuming there is no layout changes (Always assume NHWC or UNKNOWN) assert(tgraph.layout() != ir::Layout::NCHW); // TODO Different shape of back propagation tensor ir::OperandInfo backend_info{obj.shape(), obj.typeInfo(), obj.info().memAllocType(), obj.isConstant()}; tensor_builder->registerBackwardTensorInfo(ind, backend_info, ir::Layout::NHWC); }); // TODO Plan tensor builds to reduce peak memory usage tgraph.operands().iterate([&](const ir::OperandIndex &ind, const ir::Operand &) { if (tensor_builder->isRegisteredBackward(ind)) tensor_builder->notifyBackwardFirstUse(ind); }); tensor_builder->allocateBackward(); return _tensor_registry.get(); }

Therefore, at the rank-4 tensor in cker, it would be handled as NHWC.

I fixed words from specific width/height/depth to abstract(less-specific) row/col/plain. This means that CHW/HWC is not important point in cker.

Aha, I think I also got it.
Thank you 👍

YongseopKim · 2024-01-26T00:56:18Z

PTAL @jyoungyun and @zetwhite

zetwhite

LGTM 👍

YongseopKim · 2024-01-26T02:12:31Z

PTAL @jyoungyun

jyoungyun

LGTM

YongseopKim · 2024-01-26T05:02:18Z

ping @Samsung/one_onert

[cker/train] Introduce Pad op in train

bf0356c

This commit introduces Pad op in train. ONE-DCO-1.0-Signed-off-by: Yongseop Kim <[email protected]>

jyoungyun reviewed Jan 24, 2024

View reviewed changes

Apply format.patch

e8985be

YongseopKim commented Jan 24, 2024

View reviewed changes

YongseopKim requested review from jyoungyun and a team January 24, 2024 23:55

YongseopKim force-pushed the pr/cker/train/pad branch from a2271f0 to ff7b425 Compare January 24, 2024 23:57

Remove train pad function

b04c37a

YongseopKim force-pushed the pr/cker/train/pad branch from ff7b425 to b04c37a Compare January 24, 2024 23:58

YongseopKim mentioned this pull request Jan 25, 2024

draft for trainable pad op #12499

Closed

jyoungyun previously approved these changes Jan 25, 2024

View reviewed changes

jyoungyun requested a review from a team January 25, 2024 01:04

zetwhite reviewed Jan 25, 2024

View reviewed changes

compute/cker/include/cker/train/operation/Pad.h Outdated Show resolved Hide resolved

Append assertion check code

7d498ea

YongseopKim dismissed jyoungyun’s stale review via 7d498ea January 25, 2024 06:00

zetwhite reviewed Jan 25, 2024

View reviewed changes

Change for logical NHWC

74c9693

zetwhite approved these changes Jan 26, 2024

View reviewed changes

jyoungyun approved these changes Jan 26, 2024

View reviewed changes

hseok-oh merged commit 3e8d9f9 into Samsung:master Jan 26, 2024
9 checks passed

YongseopKim deleted the pr/cker/train/pad branch January 29, 2024 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cker/train] Introduce Pad op in train #12522

[cker/train] Introduce Pad op in train #12522

YongseopKim commented Jan 24, 2024 •

edited

Loading

jyoungyun Jan 24, 2024

YongseopKim Jan 24, 2024

YongseopKim Jan 24, 2024

YongseopKim Jan 24, 2024

YongseopKim commented Jan 24, 2024

jyoungyun left a comment

jyoungyun Jan 25, 2024

YongseopKim commented Jan 25, 2024

zetwhite Jan 25, 2024 •

edited

Loading

YongseopKim Jan 25, 2024

YongseopKim Jan 25, 2024

zetwhite Jan 25, 2024 •

edited

Loading

YongseopKim Jan 26, 2024

zetwhite Jan 26, 2024

YongseopKim commented Jan 26, 2024

zetwhite left a comment

YongseopKim commented Jan 26, 2024

jyoungyun left a comment

YongseopKim commented Jan 26, 2024

[cker/train] Introduce Pad op in train #12522

[cker/train] Introduce Pad op in train #12522

Conversation

YongseopKim commented Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongseopKim commented Jan 24, 2024

jyoungyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongseopKim commented Jan 25, 2024

zetwhite Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetwhite Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongseopKim commented Jan 26, 2024

zetwhite left a comment

Choose a reason for hiding this comment

YongseopKim commented Jan 26, 2024

jyoungyun left a comment

Choose a reason for hiding this comment

YongseopKim commented Jan 26, 2024

YongseopKim commented Jan 24, 2024 •

edited

Loading

zetwhite Jan 25, 2024 •

edited

Loading

zetwhite Jan 25, 2024 •

edited

Loading