[Hexagon] Resolve breakage in test_hexagon/test_cache_read_write #10520
Conversation
Breakage was caused by apache#9727, which didn't account for the new `builtin::mem_copy()` when computing the stack size in `StackSizeChecker`.
Lunderberg
requested review from
junrushao,
vinx13,
tqchen,
kparzysz-quic,
ZihengJiang,
masahi,
Hzfengsy,
comaniac,
jroesch,
areusch,
yzhliu,
merrymercy and
icemelon
as code owners
| @@ -73,10 +84,19 @@ class StackSizeChecker : public StmtExprVisitor { | |||
| return MakeShape(op); | |||
| } else if (op->op.same_as(builtin::tvm_stack_make_array())) { | |||
| return MakeArray(op); | |||
| } else if (op->op.same_as(builtin::mem_copy())) { | |||
| return MakeMemCopy(op); | |||
There was a problem hiding this comment.
Can't you just call MakeCallPacked for this?
There was a problem hiding this comment.
Not quite, because the additional string for the name of the PackedFunc to execute takes up another spot on the stack. But since I needed to test it in order to convince myself of that, I've added a comment to explain the reasoning.
There was a problem hiding this comment.
I also figured that by having the MakeMemCopyHelper be shared between StackSizeChecker and BuiltInLower makes it harder for them to get out of sync in the future.
There was a problem hiding this comment.
That makes sense. Does that mean that the name "nonexistent_function" is actually significant in the sense that its length equals or exceeds the length of the actually called function name? That would be worth a comment (if that's the case).
There was a problem hiding this comment.
Good question to ask, and no, the function name isn't significant. The stack size here is in number of arguments, where a string only counts as a single argument.
|
Also, have you tried running this test on simulator? That ability was just added over the weekend. |
|
I haven't tried running it on the simulator, no. Is there a good entry point or example for how to call into it? |
|
I think the reason that CI didn't catch this is because we don't run this test in CI. We need fix this in both of these files: @Lunderberg can you add it to this PR? |
|
@Lunderberg To run in the simulator, use the |
|
Thank you, and I can run the change in the simulator, and have updated the ci scripts to include everything in the |
|
After a discussion with @adstraw , updated the fix here to remove the |
|
Still LGTM with latest changes and I can confirm the test in questions (test_cache_read_write.py) is passing locally. |
…che#10520) * [Hexagon] Resolve breakage in test_hexagon/test_cache_read_write Breakage was caused by apache#9727, which didn't account for the new `builtin::mem_copy()` when computing the stack size in `StackSizeChecker`. * Added comment indicating need for StackSizeChecker::MakeMemCopy. * Updated unittests to run all contrib/test_hexagon at CI. * CI bump * Fix lint formatting error. * Updated fix to remove StackSizeChecker entirely. * Bugfix, verify the precheck's allocations, not own. * Bugfix, pass context information to the precheck.
Breakage was caused by #9727, which didn't account for the new
builtin::mem_copy()when computing the stack size inStackSizeChecker. This wasn't caught in CI, because the test requires the hexagon toolchain/hardware to be present.