-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[AIX] test-suites failure fixes #25791
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
base: main
Are you sure you want to change the base?
Conversation
@snnn @tianleiwu @yuslepukhin Also, please trigger CI workflows for this PR. |
@@ -362,11 +362,13 @@ void ConvertRawDataInTensorProto(TensorProto& tensor) { | |||
case TensorProto_DataType_INT16: | |||
case TensorProto_DataType_FLOAT16: | |||
case TensorProto_DataType_BFLOAT16: | |||
bytes = tensor.mutable_int32_data()->mutable_data(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be incorrect. All 4 bytes in should be swapped for a single 32 bit, not just 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuslepukhin
Some scenarios, I see where sizeof() is used while setting the raw data .
If we consider those case, then we need to consider 2 only as sizeof(BFLOAT16) is 2.
-
Do we need to consider both type of handling in this method.
One for data added by set_raw_data() and other for data added by add_int32_data types of API's. -
Or since this method is only for handling raw data, Can we first check if raw data is present or not and return if not. Not sure why we haven't added during first time, so there could be some reason and need to test it out.
Let me know your input on this.
Below is one example of test case where we use sizeof() for setting the raw data.
template <typename T>
void TestInitializerRawData() {
std::vector<T> data = GetInitializerData<T>();
ONNX_NAMESPACE::TensorProto tensor_proto;
tensor_proto.set_data_type(GetTensorProtoDataType<T>());
tensor_proto.set_name("OptimizerInitializerTest_RawData");
tensor_proto.add_dims(3);
tensor_proto.add_dims(4);
utils::SetRawDataInTensorProto(tensor_proto, data.data(), data.size() * sizeof(T));
const Initializer init(tensor_proto, std::filesystem::path());
for (size_t idx = 0; idx < data.size(); idx++) {
EXPECT_EQ(data[idx], init.data<T>()[idx]);
}
}
TEST(OptimizerInitializerTest, RawData) {
TestInitializerRawData<int8_t>();
TestInitializerRawData<uint8_t>();
TestInitializerRawData<int32_t>();
TestInitializerRawData<int64_t>();
TestInitializerRawData<MLFloat16>();
TestInitializerRawData<BFloat16>();
TestInitializerRawData<float>();
TestInitializerRawData<double>();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the data is stored as raw data, and the size of the type is 2 bytes, then they are stored contigiously and for that sizeof() needs to be 2 bytes.
However, if it is not stored as raw data, and in this case stored in int32, then all 4 bytes need to be swapped, although only two bytes are meaningful.
Thus we either need two different switches, branch on the presense of raw data and set the sizeof() accordingly OR we need two different sizes for raw data and otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I will put two different switches based on raw data presence. Let me test it out.
Description
This PR is to fix some of the test case failures mentioned in #25790
To fix the onnxruntime_shared_lib_test crash
For fixing , EpGraphTest.SerializeToProto_ConstantOfShape
For fixing many test failures , related to float16 or Int16 type.
For fixing, ConvertRawDataInTensorProtoTest.FloatData and ConvertRawDataInTensorProtoTest.Int32Data
Motivation and Context
To fix the AIX (Big endian) related test failures.