Fix function number datatype from char to uint16_t by A1245967 · Pull Request #10014 · apache/tvm

A1245967 · 2022-01-21T13:54:25Z

Hi, @areusch
Here is the bug I mentioned before.
I cast the first two byte to uint16_t to store the function number.

comaniac

LGTM

comaniac · 2022-01-21T18:10:35Z

  reg->registry.names = (const char*)buffer;
  buffer[0] = 0;  // number of functions present in buffer.
-  buffer[1] = 0;  // end of names list marker.
+  buffer[1] = 0;


Better to explicitly mention the purpose. Something like

Suggested change

buffer[1] = 0;

buffer[1] = 0; // note that we combine the first two elements to form a 16-bit function index.

kparzysz-quic

There are other places in this file that rely on the "one char for function count", that this patch does not update.

The approach of hiding the number of functions in the "names" buffer is dangerous---sooner or later somebody will forget it, or miss some cases, and it will result in hard-to-find bugs. I think it would be better to make it explicit in the data structures, or to create special functions that get/update the function count and get the actual beginning of the function names.

comaniac · 2022-01-21T18:36:57Z

@kparzysz-quic thanks for pointing out the missing pieces. For the discussion about putting number in the name buffer, please refer to the discussion: https://discuss.tvm.apache.org/t/byoc-limited-function-numbers-on-crt-runtime/11952

areusch · 2022-01-23T21:55:46Z

@kparzysz-quic i agree it can be a bit misleading. if you feel strongly about it, i'm okay with us adopting the more explicit approach. it would waste a few bytes, but we do now provide a more compact solution for the standalone deployment workflow, so perhaps that's okay now. i am curious on your take on my rationale presented in the thread Cody links above. Regardless of the strategy taken around the C struct, it's going to be a bit dangerous since we generate this structure from LLVM and C source codegen, and then try to re-interpret the structure using a different compiler.

@A1245967 you'll also need to update the LLVM codegen and C source codegen to generate the proper structure.

kparzysz-quic · 2022-01-24T00:25:36Z

I agree that accessing C structures via LLVM codegen may be tricky (due to pointer sizes in particular, and alignment rules), we're already running into a similar issue with enums in Device (IIRC). It should be ok to have the number of functions "hidden" in the buffer, but we should have separate functions to (1) read it from the buffer, (2) store it to the buffer, (3) get the beginning of the actual payload. This is what I meant in my first comment---this way we would avoid things like for (char *p = names + 1; ...), where it's not clear what that + 1 means, and instead have something like for (char *p = some_better_name_for_the_beginning_of_the_actual_names(); ...). This would localize the encoding and decoding of the function count to specialized function, where the situation can be explained in comments. This is all to make it harder for people to make an accidental mistake.

areusch · 2022-04-08T19:47:44Z

@kparzysz-quic ah i understand. sure, that's totally fine with me.

@A1245967 , if you're up for it, would you mind introducing a few helper functions e.g. TVMFuncRegistry_GetNumFuncs and TVMFuncRegistry_Get0thFunctionName and updating the remaining places to match? i think we can merge with that.

A1245967 · 2022-04-09T16:20:02Z

@areusch okay, I try to use some helper functions instead of accessing the buffer directly.

areusch · 2022-04-11T18:18:35Z

@A1245967 that looks pretty good--i think you just need to update the C++ unit tests in tests/crt/func_registry_test.cc

A1245967 · 2022-04-20T10:21:35Z

@areusch I have updated the code. Do I need to modify anything else?

areusch

ahh--sorry @A1245967 i found one more problem. could you address this one, and then i think we're good to merge.

areusch · 2022-04-20T23:34:31Z

  return return_value;
 }

+uint16_t* TVMFuncRegistry_GetNumFuncs(const TVMFuncRegistry* reg) { return (uint16_t*)reg->names; }


for this function, i think we need to impl like this to avoid alignment problems:

Suggested change

uint16_t* TVMFuncRegistry_GetNumFuncs(const TVMFuncRegistry* reg) { return (uint16_t*)reg->names; }

uint16_t TVMFuncRegistry_GetNumFuncs(const TVMFuncRegistry* reg) {

uint16_t num_funcs;

memcpy(&num_funcs, reg->names, sizeof(num_funcs));

return num_funcs;

}

I think the following line also need to be changed if I choose to return the value instead of the address.

We update the value of num_funcs at this line .
If I choose to return the value in TVMFuncRegistry_GetNumFuncs, I should write a function to update num_funcs in buffer.
Do I need to write a function like TVMFuncRegistry_SetNumFuncs to set the value of num_funcs?

ah, yeah i think so. good catch.

@areusch I have implemented the functions TVMFuncRegistry_SetNumFuncs and TVMFuncRegistry_GetNumFuncs.
You can review these functions.

rewrite the modified part to pass lint check Use 2 bytes for func num in fun_registry Fix errors in linter Add the declaration of the helper functions set 2 bytes for func num in func_registry test units pass num_func by value This commit change the datatype of the number of the function from 1 Byte to 2 Bytes. Besides, I use some helper functions to access the number of function and the first function name.

areusch

thanks @A1245967 !

areusch · 2022-05-18T18:58:52Z

@kparzysz-quic merged this now

…10014)" (#11363) This reverts commit f34bd22. Co-authored-by: driazati <driazati@users.noreply.github.com>

driazati · 2022-05-18T22:22:16Z

Hey @A1245967 this PR's last CI run was a bit out of date so it broke things when it was merged with the main branch, so we had to revert it in c32224f. If you open another PR with the same changes (which you can do with something like

cd tvm
git fetch origin main
git reset --hard origin/main
curl -L https://github.com/apache/tvm/pull/10014.diff | patch -p1 -N -d .
# then git add, commit as usual

) and fix the CI bug we can get it merged back in ASAP

A1245967 requested review from areusch, liangfu, manupak and tmoreau89 as code owners January 21, 2022 13:54

comaniac approved these changes Jan 21, 2022

View reviewed changes

kparzysz-quic reviewed Jan 21, 2022

View reviewed changes

areusch reviewed Apr 20, 2022

View reviewed changes

areusch approved these changes May 11, 2022

View reviewed changes

areusch merged commit f34bd22 into apache:main May 18, 2022

driazati mentioned this pull request May 18, 2022

[skip ci] Revert "Fix function number datatype from char to uint16_t (#10014)" #11363

Merged

AndrewZhaoLuo pushed a commit that referenced this pull request May 18, 2022

[skip ci] Revert "Fix function number datatype from char to uint16_t (#…

c32224f

…10014)" (#11363) This reverts commit f34bd22. Co-authored-by: driazati <driazati@users.noreply.github.com>

areusch mentioned this pull request May 18, 2022

Fix function number datatype from char to uint16_t #11365

Merged

A1245967 mentioned this pull request May 18, 2022

Fix function number datatype from char to uint16_t #11366

Closed

	buffer[1] = 0;
	buffer[1] = 0; // note that we combine the first two elements to form a 16-bit function index.

-uint16_t* TVMFuncRegistry_GetNumFuncs(const TVMFuncRegistry* reg) { return (uint16_t*)reg->names; }
+uint16_t TVMFuncRegistry_GetNumFuncs(const TVMFuncRegistry* reg) {
+  uint16_t num_funcs;
+  memcpy(&num_funcs, reg->names, sizeof(num_funcs));
+  return num_funcs;
+  }

Uh oh!

Conversation

A1245967 commented Jan 21, 2022

Uh oh!

comaniac left a comment

Choose a reason for hiding this comment

Uh oh!

comaniac Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

kparzysz-quic left a comment

Choose a reason for hiding this comment

Uh oh!

comaniac commented Jan 21, 2022

Uh oh!

areusch commented Jan 23, 2022

Uh oh!

kparzysz-quic commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

areusch commented Apr 8, 2022

Uh oh!

A1245967 commented Apr 9, 2022

Uh oh!

areusch commented Apr 11, 2022

Uh oh!

A1245967 commented Apr 20, 2022

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

areusch Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

A1245967 Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

areusch Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

A1245967 May 9, 2022

Choose a reason for hiding this comment

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

areusch commented May 18, 2022

Uh oh!

driazati commented May 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kparzysz-quic commented Jan 24, 2022 •

edited

Loading