Aharrypotter commented on code in PR #19814:
URL: https://github.com/apache/tvm/pull/19814#discussion_r3427549698


##########
python/tvm/relax/frontend/tflite/tflite_frontend.py:
##########
@@ -5009,7 +5009,22 @@ def convert_reverse_sequence(self, op):
         batch_axis = options.BatchDim()
         seq_axis = options.SeqDim()
 
-        return relax.op.reverse_sequence(in_expr, length_expr, seq_axis, 
batch_axis)
+        if batch_axis != 0:
+            raise tvm.error.OpNotImplemented(
+                "TFLite REVERSE_SEQUENCE with non-zero batch_dim is not 
supported yet."
+            )
+
+        output_tensors = self.get_output_tensors(op)
+        assert len(output_tensors) == 1, "output tensors length should be 1"
+        output_tensor = output_tensors[0]
+        output_shape = to_int_list(self.get_tensor_shape(output_tensor))
+        output_dtype = self.get_tensor_type_str(output_tensor.tensor.Type())
+
+        return relax.op.call_dps_packed(
+            "topi.reverse_sequence",
+            (in_expr, length_expr, seq_axis),
+            out_sinfo=relax.TensorStructInfo(output_shape, output_dtype),
+        )

Review Comment:
   Thanks for the careful check. I looked at the current packed registration in
   `src/topi/transform.cc`, and `topi.reverse_sequence` is registered as a
   3-argument packed function:
   
   ```cpp
   .def_packed("topi.reverse_sequence",
               [](ffi::PackedArgs args, ffi::Any* rv) {
                 *rv = reverse_sequence(args[0].cast<te::Tensor>(),
                                        args[1].cast<te::Tensor>(),
                                        args[2].cast<int>());
               })
   ```
   
   So passing `(data, seq_lengths, seq_axis, batch_axis)` from Relax would not 
match
   the current packed API. The frontend already rejects `batch_dim != 0` before 
the
   call, and the 3-argument packed wrapper therefore intentionally uses TOPI's
   default `batch_axis=0`.
   
   I think the current 3-argument call and the matching Expected IR are the 
safer
   form for this PR. If we want to support non-zero `batch_dim` later, the right
   follow-up would be to first extend the `topi.reverse_sequence` packed
   registration to consume `args[3]`, and then update the TFLite frontend and 
test
   expectation to pass `batch_axis` explicitly.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to