[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Apologies, missed a couple of small things out. Otherwise looks good to me.



================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:150
 
+// The backend does not have support for hard thread pointers when targeting
+// Thumb1.
----------------
peter.smith wrote:
> Will be worth saying `CP15 C13 ThreadID` somewhere to make it easier to look up in the documentation.
Now we're permitting anything with the co-processor instructions I think the comment would be better expressed as:
``` We follow GCC and support when the backend has support for the MRC/MCR instructions that are used to set the hard thread pointer ("CP15 C13 //Thread id").```


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName());
+  return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 ||
+         (Ver == 6 && Triple.isARM());
----------------
peter.smith wrote:
> nickdesaulniers wrote:
> > peter.smith wrote:
> > > nickdesaulniers wrote:
> > > > ardb wrote:
> > > > > peter.smith wrote:
> > > > > > Are we restricting based on whether the threadid register is present or whether the instructions are available to access the cp15 registers?
> > > > > > 
> > > > > > If we're going by whether the CPU has the register present then it will be
> > > > > > * A and R profile (not M profile, even the ones that have Thumb2)
> > > > > > * V6K (includes ARM1176 but not ARM1156t2-s which has Thumb-2!) and V7+ (A and R profile)
> > > > > > 
> > > > > > If we're going by the instructions to write to CP15 then it is:
> > > > > > * Arm state (everything)
> > > > > > * Thumb2 (v7 + v6t2)
> > > > > > 
> > > > > > The above seems to be a blend of the two. Is it worth choosing one form or the other? GCC seems to use the latter. I guess using this option falls into the I know what I'm doing area that accessing named system registers comes into. If the kernel supports it the stricter version may help catch more mistakes though.
> > > > > > 
> > > > > > The v7 A/R Arm ARM https://developer.arm.com/documentation/ddi0403/ed
> > > > > > has in `D12.7.21 CP15 c13, Context ID support`
> > > > > > ``` An ARMv6K implementation requires the Software Thread ID registers described in VMSA CP15 c13
> > > > > > register summary, Process, context and thread ID registers on page B3-1474. ```
> > > > > > 
> > > > > > The Arm 1156-s (the only v6t2 processor) TRM https://developer.arm.com/documentation/ddi0338/g/system-control-coprocessor/system-control-processor-registers/c13--process-id-register?lang=en which shows only one process ID register under opcode 1 accessed via:
> > > > > > ```
> > > > > > MRC p15, 0, <Rd>, c13, c0, 1 ;Read Process ID Register
> > > > > > ```
> > > > > > Whereas the ThreadID register is opcode 3 on CPUs that are v6k and v7.
> > > > > The primary reason for tightening these checks was to avoid an assert in the backend when using -mtp=cp15 with a Thumb1 target, so I'd say this is more about whether the ISA has the opcode to begin with, rather than whether CPU x implements it or not.
> > > > > Arm state (everything)
> > > > 
> > > > 
> > > > Does that mean that `-march=armv5 -marm` has access/support for "CP15 C13 ThreadID" access?
> > > The co-processor instructions are the same, but the effect of the instructions will depend on the CPU. For example on armv5 we can write the instruction to read/write to CP15 C13 with the ThreadID opcode. However on no armv5 implementation will the CP15 C13 have a Thread ID register. 
> > > 
> > > The more complex stricter check will make sure that the implementations have the ThreadID register. As Ard mentions, the GCC intent seems to be whether the instruction is encodable rather than check what the CPU supports. 
> > I think this thread is resolved, so marking as done. Please reopen it though if I misunderstood.
> I think the discussion about whether the MRC/MCR instructions are available is closed. I think the expression isn't quite right as > 7 will permit v8-m.baseline, which is the evolution of cortex-m0.
> 
> To match GCC I think you want the equivalent of ARM || hasThumb2Instructions() 
> 
> I think you can do that with the equivalent of ArmTargetInfo::supportsThumb2() from lib/Basic/Targets/ARM.cpp
> ```
> bool ARMTargetInfo::supportsThumb2() const {
>   return CPUAttr.equals("6T2") ||
>          (ArchVersion >= 7 && !CPUAttr.equals("8M_BASE"));
> }
> ```
> Where CPUAttr.equals("8M_BASE") is equivalent to `llvm::ARM::ArchKind::ARMV8MBaseline` so I think you can use
> ```
> return Triple.isARM() || (Ver >= 7 && AK != llvm::ARM::ArchKind::ARMV8MBaseline);
> ```
> 
> ARMV6K and ARMV6KZ can only MRC and MCR in ARM mode so are covered by Triple.isARM().
> 
> 
My apologies I missed out V6T2 in my comment above. I've added it back in below.
```
return Triple.isARM() || AK ==  llvm::ARM::ArchKind::ARMV6T2 || (Ver >= 7 && AK != llvm::ARM::ArchKind::ARMV8MBaseline);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114116/new/

https://reviews.llvm.org/D114116