[PATCH] D23931: [XRay] ARM 32-bit no-Thumb support in LLVM

[Serge Rogatch via llvm-commits]

rSerge added a comment.

In https://reviews.llvm.org/D23931#530369, @rengolin wrote:

> In https://reviews.llvm.org/D23931#530324, @rSerge wrote:
>
> > I'm not targeting all ARM architectures at once, at least not in the first commit. I think we should choose 1 ARM architecture for which XRay works, and assume the others not supported or experimental. Currently I am building and experimenting with `armhf` (32-bit).
>
>
> Right, "armhf" is not one ARM architecture, but dozens. It can be anything from v6T2 to V8.2A, including all sub-architectures, features and variations. Though, from what I've seen so far, the code you use would work on any architecture of that range.


Thanks for explaining. I am still starting with ARM and LLVM.

> It would be safer, though, to document the *intended* target specifically, like "ARMv7A with VFPv3 support". So that people with "ARMv6T2 with VFPv2" support are not surprised when you assumed something "wrong" for them. Adding a "hasV6T2Ops()" check on the entry-point would help.


Ok, I'll try to select something more specific than `armhf`.

> 

> 

> > Sled sizes do not have to fit all architectures either (this would result in waste of space for some, thus worse performance due to cache misses). Currently sleds are 11 bytes on x86_64 and 28 bytes on `armhf`.

> 

> 

> Check.

> 

> > What is PCS ?

> 

> 

> Procedure Call Standard. This is the part of the ABI that defines how functions are called to be compatible with the ABI. Mostly about how to serialise arguments and return values in registers, stack, etc.

> 

> Both C and C++, as well as any other language that wants to be compatible with ARM's EABI standard *have* to abide to those terms.

> 

> > Thumb is not supported yet.

> 

> 

> Do you mean not supported in the Sled code, or inserting ARM Sled code into Thumb functions?


Neither is supported. I estimated that Thumb support requires substantial additional effort.

> If the former, then you have to check if the architecture/OS/ABI you're supporting allows ARM code. For instance, Windows doesn't.

> 

> If the latter, than you need to check if the architecture/OS/ABI you're supporting allows Thumb code. For instance, there could be libraries around, or even inline assembly with ".thumb" in it (yes, this does happen). I can't remember how, but there's a way to know what's the ISA for a specific function, this could help you. OTOH, this could be an assembler things, can't remember.


Yes, this looks like a lot of effort.

> Any way, you need to check if the architecture/OS/ISA/ABI you have is compatible with your assumptions before you emit code.

> 

> > Architectures which do not support BLX are not supported.

> 

> 

> Fair enough. But as I said earlier, this has to be clearly encoded (via error messages) on the entry-point of your code.

> 

> > Any evidence that `POP {lr}` is deprecated?

> 

> 

> Sorry, my bad. I was thinking about a different case. Ignore me.

> 

> > Any specific examples which more tests should be added for the single supported architecture `armhf`?

> 

> 

> As I said earlier, you need to make sure you only emit your stubs on architectures that you know works. Checking the target for architecture level, ISA support and ABI should be enough, at least on the entry-point.

> 

> Adding tests is, then, easily done by having two files: one where everything should fail, RUNning with a "not" before "llc", CHECKing for error messages, and one where everything should pass, CHECKing for the correct sequence of Nops, etc.

> 

> It should be fine to add all error messages to one file and all cases that should pass to another.

> 

> cheers,

> --renato


The amount of change requested in the code review seems too much for the first iteration. Can we limit the scope and plan incremental improvements?

Cheers,
Serge


https://reviews.llvm.org/D23931