This is a port of XRay to ARM 32-bit, without Thumb support yet.
This is one of 3 commits to different repositories of XRay ARM port. The other 2 are:
- https://reviews.llvm.org/D23931 (LLVM)
- https://reviews.llvm.org/D23932 (Clang test)
Differential D23933
[XRay] ARM 32-bit no-Thumb support in compiler-rt rSerge on Aug 26 2016, 10:21 AM. Authored by
Details
This is a port of XRay to ARM 32-bit, without Thumb support yet.
Diff Detail
Event TimelineComment Actions Rebased after https://reviews.llvm.org/D21982 and ported logging to ARM: replaced RDTSC instruction with clock_gettime().
Comment Actions Please, see my responses inline. I'll upload the updated patch in a few minutes.
Comment Actions I'll defer to someone else who understands the ARM assembly parts. You might also consider extending the file header type to indicate what platform the trace was generated from, so tools can determine what to do with a trace that comes from a specific CPU. I'm happy to do it later, but wanted to know your thoughts on how we might encode that information appropriately.
Comment Actions Sorry, it was holiday in UK today... I'll have a look at the patches tomorrow. Cheers, Comment Actions Hi, I have a number of comments and requests. One general remark, also, is to comment on the top of the assembly functions what's the function signature in C, so that I know how to review the function's code. Otherwise, it's very hard to understand all possibilities. cheers,
Comment Actions I've responded inline and will upload a new diff in a minute.
I think we should look for some enumeration in compiler-rt or LLVM listing the CPU architectures, so to make XRay CPU codes consistent with that. But so far XRayFileHeader and XRayRecord don't seem to differ between CPUs, or do they?
Comment Actions Thanks for the changes, some more comments...
Comment Actions Fixed Ubuntu x86_64 build. Implemented the changes requested in code review comments. Comment Actions This looks good to me, thanks for all the changes! If @dberris is happy, I'm happy. :) cheers, PS: I may have missed a few things, but we can fix as we go, when support gets better. Comment Actions Rebased to the latest revision. I don't have commit access rights. Could someone commit? Comment Actions So, you're forcing vfpv3, which is armv7-only. AFAICS, you're only using VPUSH and VPOP, which is available since vfpv2 (which is also available in v6), so maybe a better fix would be to use: .arch armv6t2 .fpu vfpv2 which should work on armv7, too. Since this is the restriction we have inside the code, it would be more clear this way. Can you do a quick test with those directives? cheers, Comment Actions The test is to make sure it won't break the bots again. Should be quick, as he had done it before. Comment Actions armv6t2 shouldn't work because MOVW and MOVT instructions are available only since armv7 .
Comment Actions Movw/Movt are Thumb2 instructions and were introduced in ARMv6T2.
I'm not worried about the assembly code working on v6T2 or VFPv2, I'm worried about the toolchain coping with the options. You just need to get the complete command line with a recent enough cross-toolchain (4.8+) and try on the resulting file. cheers, PS: You should really get the ARM ARMs: http://llvm.org/docs/CompilerWriterInfo.html
Comment Actions I've tested .arch armv6t2 .fpu vfpv2 with Clang cross-compiling from x86_64-Windows to ARM-Linux and Thumb-Linux, and GCC cross-compiling from x86_64-Ubuntu to ARM-Linux and Thumb-Linux. No compile errors so far.
Comment Actions Sorry, I wasn't clear. These two lines should work on any toolchain, my point is if that makes it break with your gnu toolchain because of the same issue (minimal ISA support assumed) in *conjunction* with the rest of the code. Comment Actions Perfect, let's try again. :) @dberris, you have already committed the other two patches again, right? Would you do the honours? |