Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT][dredd] Introduce dredd test for TensorShape #14050

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion compiler/circle-inspect/driver/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ int entry(int argc, char **argv)
arser.add_argument("--constants").nargs(0).help("Dump constant tensors name");
arser.add_argument("--op_version").nargs(0).help("Dump versions of the operators in circle file");
arser.add_argument("--tensor_dtype").nargs(0).help("Dump dtype of tensors");
arser.add_argument("--tensor_shape").nargs(0).help("Dump shape of tensors");
arser.add_argument("circle").help("Circle file to inspect");

try
Expand All @@ -51,7 +52,7 @@ int entry(int argc, char **argv)
}

if (!arser["--operators"] && !arser["--conv2d_weight"] && !arser["--op_version"] &&
!arser["--tensor_dtype"] && !arser["--constants"])
!arser["--tensor_dtype"] && !arser["--constants"] && !arser["--tensor_shape"])
{
std::cout << "At least one option must be specified" << std::endl;
std::cout << arser;
Expand All @@ -70,6 +71,8 @@ int entry(int argc, char **argv)
dumps.push_back(std::make_unique<circleinspect::DumpTensorDType>());
if (arser["--constants"])
dumps.push_back(std::make_unique<circleinspect::DumpConstants>());
if (arser["--tensor_shape"])
dumps.push_back(std::make_unique<circleinspect::DumpTensorShape>());

std::string model_file = arser.get<std::string>("circle");

Expand Down
35 changes: 35 additions & 0 deletions compiler/circle-inspect/src/Dump.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -240,3 +240,38 @@ void DumpConstants::run(std::ostream &os, const circle::Model *model, const std:
}

} // namespace circleinspect

namespace circleinspect
{

void DumpTensorShape::run(std::ostream &os, const circle::Model *model,
const std::vector<char> *data)
{
mio::circle::Reader reader(model, data);

const uint32_t subgraph_size = reader.num_subgraph();

for (uint32_t g = 0; g < subgraph_size; g++)
{
reader.select_subgraph(g);
auto tensors = reader.tensors();

for (uint32_t i = 0; i < tensors->size(); ++i)
{
const auto tensor = tensors->Get(i);
auto shape = tensor->shape_signature() ? tensor->shape_signature() : tensor->shape();
os << reader.tensor_name(tensor) << " ";
for (int8_t i = 0; i < shape->size(); i++)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a must, but most machines do calculations in 32-bit units, so there's probably no need to reduce it to 8-bit in for loop.

Suggested change
for (int8_t i = 0; i < shape->size(); i++)
for (uint32_t i = 0; i < shape->size(); i++)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a must, but most machines do calculations in 32-bit units, so there's probably no need to reduce it to 8-bit in for loop.

I'll update it.

{
os << shape->Get(i);
if (i != shape->size() - 1)
{
os << ",";
}
}
os << std::endl;
}
}
}

} // namespace circleinspect
9 changes: 9 additions & 0 deletions compiler/circle-inspect/src/Dump.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@ class DumpConstants final : public DumpInterface
void run(std::ostream &os, const circle::Model *model, const std::vector<char> *data);
};

class DumpTensorShape final : public DumpInterface
{
public:
DumpTensorShape() = default;

public:
void run(std::ostream &os, const circle::Model *model, const std::vector<char> *data);
};

} // namespace circleinspect

#endif // __DUMP_H__
1 change: 1 addition & 0 deletions compiler/circle2circle-dredd-recipe-test/test.lst
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ Add(PadV2_001 PASS substitute_padv2_to_pad)
Add(Softmax_001 PASS decompose_softmax)
Add(Softmax_002 PASS decompose_softmax)
Add(StridedSlice_003 PASS substitute_strided_slice_to_reshape)
Add(Inf_Pad_000 PASS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sort in alphabetical order

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sort in alphabetical order

Instead of sorting, what about separate Inf_ tests?

Suggested change
Add(Inf_Pad_000 PASS)
# Shape Inference test
Add(Inf_Pad_000 PASS)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, what about separate Inf_ tests?

👍


# CSE test

Expand Down
17 changes: 17 additions & 0 deletions compiler/dredd-rule-lib/rule-lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -252,4 +252,21 @@ const_count()
echo ${ACTUAL}
}

tensor_shape()
{
argc_check $# 1
file_path_check ${COMPILED_FILE}
file_path_check ${INSPECT_PROG_PATH}

set -o pipefail

ACTUAL=`init_error_log ; \
${INSPECT_PROG_PATH} --tensor_shape ${COMPILED_FILE} | \
awk -v tensor_name="$1" '{ if ($1 == tensor_name) print $2}'`

check_success_exit_code $? 0

echo ${ACTUAL}
}

# TODO define more qullity test function
33 changes: 33 additions & 0 deletions res/TensorFlowLiteRecipes/Inf_Pad_000/test.recipe
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# padding with dynamic shape, others same as Pad_000
operand {
name: "ifm"
type: FLOAT32
shape { dim: 1 dim: 1 dim: 3 dim: 2 }
shape_signature { dim: 1 dim: -1 dim: 3 dim: 2 }
}
operand {
name: "padding"
type: INT64
shape { dim: 4 dim: 2 }
filler {
tag: "explicit"
arg: "0" arg: "0"
arg: "1" arg: "1"
arg: "2" arg: "2"
arg: "0" arg: "0"
}
}
operand {
name: "ofm"
type: FLOAT32
shape { dim: 1 dim: 1 dim: 7 dim: 2 }
shape_signature { dim: 1 dim: -1 dim: 7 dim: 2 }
}
operation {
type: "Pad"
input: "ifm"
input: "padding"
output: "ofm"
}
input: "ifm"
output: "ofm"
6 changes: 6 additions & 0 deletions res/TensorFlowLiteRecipes/Inf_Pad_000/test.rule
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# To check if dynamic dimension properly infered
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo
=)

Suggested change
# To check if dynamic dimension properly infered
# To check if dynamic dimension properly inferred

Copy link
Contributor Author

@icodo98 icodo98 Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo =)

Sorry, I'll fix it.


RULE "VERIFY_FILE_FORMAT" $(verify_file_format) '=' 1

RULE "PAD_EXIST" $(op_count PAD) '=' 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, PAD_EXIST is not related to SHAPE_INFERENCE.
Why is this necessary?

Copy link
Contributor Author

@icodo98 icodo98 Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

You're right.

RULE "PAD_SHAPE" $(tensor_shape ofm) '=' 1,-1,7,2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'shape' expression 1,-1,7,2 is fine now, but it would be better to express it in a more formalized form like [1, -1, 7, 2].
It would be better if it could handle spaces between commas as well.

Suggested change
RULE "PAD_SHAPE" $(tensor_shape ofm) '=' 1,-1,7,2
RULE "PAD_SHAPE" $(tensor_shape ofm) '=' [1, -1, 7, 2]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from ##13998 (comment)

FYI) RULE "PAD_SHAPE" $(tensor_shape ofm) '=' [1, -1, 7, 2] does not work with the current code. Currently, the rule only works if the arguments have exactly 4 elements, so there cannot be any spaces in between, such as [1, -1, 7, 2]. Therefore, at the moment, we are checking shapes in the form of 1,-1,7,2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks for the description.
How about using a quoted string, i.e., "[1, -1, 7, 2]"?
If it's difficult, the current implementation is not bad either =)