u99127 commented on pull request #7415: URL: https://github.com/apache/tvm/pull/7415#issuecomment-777399529
> > > @manupa-arm @grant-arm it seems to me that this change is replacing pointer dereferences to `**strm` with `memcpy`. this is the correct thing to do, so now we just need to test it. the issue now is that it compiles fine, but at runtime would trigger an unaligned load. this issue should be present across platforms. I think we could write a test that intentionally calls `TVMNDArray_Load` with unaligned `strm`. i'm not suggesting we need to test every single case of mis-alignment, but a single test that calls TVMNDArray_Load with unaligned `strm` should be sufficient to trigger the failure without this patch. Sorry to jump in but I think a couple of points need to be considered here. You need an architecture that traps on unaligned accesses. AFAIK, that's not x86_64 in these cases. And thus it is unlikely that you will provoke this AFAIK. this is one of the common cases where things go wrong in this case. AFAIK it will not trap on x86_64. This is a common problem when undefined C is ported between x86_64 and architectures that have strict alignment. My understanding is that you will not see this failure. I will point out that once we start running tests on Corstone 300 (Cortex-M + Ethos-U), note that the runtime will run on the Cortex-M and publish that upstream in a month or so, there will be a test on a target in the CI that actually breaks for this. After all if the runtime fails with misaligned faults it's fairly fundamental to running any tests. -Wcast-align=strict works well across all architectures as an option from GCC-8 ( notably it wouldn't work on x86_64 as IIRC x86_64 is not a STRICT_ALIGNMENT target in GCC parlance). IIRC the default compiler in CI is from Ubuntu 18.04 and thus will not show up on x86_64 in GCC-7 . $> gcc -Wcast-align=strict /tmp/tst.c gcc: error: unrecognized command line option ‘-Wcast-align=strict’; did you mean ‘-Wcast-align’? $> gcc -v gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04) I would suggest fixing the issue, it's undefined C . Yes it's nice to have a test but that will come automatically with testing initial support for Ethos-U55 in the Corstone300 FVP or any Cortex-M test that runs on an FVP. Thanks, Ramana > > ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
