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

Tensor changes in master graph and ring buffer #17

Merged

Conversation

fiona-gladwin
Copy link
Collaborator

  • Change all instances of Image to rocalTensor

In Master Graph

  • Removed the internal batch size related params and computations
  • Removed the output routine for video and making use of single loader routine for image and vide pipeline
  • Remove variables associated with the above changes
  • Remove output tensor allocation and deallocation (used for output copy)
  • Remove copy_data API
  • Introduced get_output_tensors API to directly return output tensor list

In Ring Buffer

  • Remove master host buffers
  • Modified host sub buffers to be used similar to device sub buffers

@LakshmiKumar23 LakshmiKumar23 requested a review from rrawther March 6, 2023 18:04
#endif
_convert_time.end();
return Status::OK;
std::vector<void*> output_ptr = _ring_buffer.get_read_buffers();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this call doing a copy or returning reference to a vector? Can you please use auto call like the image pipeline here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed this to auto. This call would return a copy of the vector of pointers

for(unsigned i = 0; i < _internal_tensor_list.size(); i++)
_output_tensor_list[i]->set_mem_handle(output_ptr[i]);

return &_output_tensor_list;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In case of framework integration: how does it copy to framework tensor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We would return the tensor to user and in the iterator we call a tensor.copy_data(user_buffer) where we pass a user buffer and the contents of tensor is copied to user buffer. This API is added as a member function of rocalTensor class in tensor.cpp

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Fionna, In that case we need to make copy_data() uses optimized code to get the best performance

{
ERR("Exception thrown in the process routine: " + STR(e.what()) + STR("\n"));
_processing = false;
_ring_buffer.release_all_blocked_calls();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this exception handling code removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have removed the output routine of the video, so it is showing this as removed but the code is present for the same

@@ -1493,163 +783,6 @@ bool MasterGraph::no_more_processed_data()
return (_output_routine_finished_processing && _ring_buffer.empty());
}

MasterGraph::Status
MasterGraph::copy_out_tensor_planar(void *out_ptr, RocalTensorFormat format, float multiplier0, float multiplier1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this was required for supporting planar output from the pipeline (like for CFar10 data set). Is this handled in tensor pipeline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes this is handled in tensor pipeline, some of our trainings require planar output i.e NCHW we are able to return a planar output from rocAL and normalization would happen in RPP.

@@ -159,7 +151,7 @@ void RingBuffer::init(RocalMemType mem_type, void *devres, unsigned sub_buffer_s

}
}
else
else
Copy link
Collaborator

Choose a reason for hiding this comment

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

make sure the code works for all 3 backends (ENABLE_HIP, ENABLE_OPENCL, CPU)

@@ -316,7 +316,7 @@ unsigned rocalTensor::copy_data(hipStream_t stream, void *host_memory, bool sync
#endif

unsigned rocalTensor::copy_data(void *user_buffer) {
if (_info._type != rocalTensorInfo::Type::HANDLE) return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this changed? Are they both have the same meaning

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In our previous case we would associate all the image related rocalTensors with a vx_tensor and we set the type as handle for the output tensors where we create vx_tensors with handle.
But with our current changes we are not creating the output tensor (which is to be returned to the user) as a vx_tensor hence handle is not set, so we could not copy the data. Hence I have changed the condition to check if it is a null pointer.

Copy link
Collaborator

@rrawther rrawther left a comment

Choose a reason for hiding this comment

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

@fiona-gladwin : please address review comments

@fiona-gladwin
Copy link
Collaborator Author

Hi @rrawther and @LakshmiKumar23 I have addressed all the PR comments.

@LakshmiKumar23
Copy link
Owner

Hi @rrawther and @LakshmiKumar23 I have addressed all the PR comments.

@rrawther can I merge this PR?

Copy link
Collaborator

@rrawther rrawther left a comment

Choose a reason for hiding this comment

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

I see that many functions for copy() are removed from this.
We need to make sure we handle all those cases in the tensor pipeline

@LakshmiKumar23 LakshmiKumar23 merged commit 006eadc into LakshmiKumar23:lk/rocalTensor Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants