nickdesaulniers added a comment.
Codegen looks good, probably just need an additional conditional on
`ARMSubtarget::isReadTPHard()`. With that and some more tests for cases we
don't intend to support (thumb[1], soft tp) this LGTM. Great job @ardb !
================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:4900
+ .addImm(15)
+ .addImm(0)
+ .addImm(13)
----------------
ardb wrote:
> nickdesaulniers wrote:
> > I think we want to use `Reg` in this instruction, in order to load into the
> > specified destination?
> We use Reg, no? MRC does not take reg operands, but I pass it as the target
> register.
ah, yep, sorry, LGTM.
================
Comment at: llvm/lib/Target/ARM/ARMInstrInfo.cpp:102
+ if (M.getStackProtectorGuard() == "tls") {
+ expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+ return;
----------------
ardb wrote:
> nickdesaulniers wrote:
> > do we have to worry about soft tp, ie. `__aeabi_read_tp` vs `mrc`?
> I think we should only allow this hard TP is supported in the first place.
> I'll add a check somewhere.
> Calling a helper in both the prologue and the epilogue for emulated TLS seems
> a bit too heavyweight to me, and the Linux kernel will not use it in that way
> anyway.
I agree. Let's add a test that this only works with the `-mattr=+read-tp-hard`
llc flag or `"target-features"="+read-tp-hard"` function attribute (one or the
other, I don't think we need to exhaustively test both). But it would be good
to verify what happens when `+read-tp-hard` is not set, in case someone else
cares to add soft tp support here in the future. (They may never, it's more so
to highlight whether such a change would be intentional).
`ARMSubtarget::isReadTPHard()` should be able to help us differentiate this
case.
================
Comment at: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp:250
void Thumb2InstrInfo::expandLoadStackGuard(
MachineBasicBlock::iterator MI) const {
----------------
ardb wrote:
> nickdesaulniers wrote:
> > what about `Thumb1InstrInfo::expandLoadStackGuard`? Do we have `mrc`
> > available in Thumb[1] as well?
> No Thumb1 does not have an encoding for MRC so we can ignore it here. It does
> mean we should not allow this feature to be turned out for such targets.
Let's add a test for such case.
================
Comment at: llvm/test/CodeGen/ARM/stack-guard-tls.ll:5-11
+; RUN: FileCheck --check-prefix=CHECK --check-prefix=CHECK-SMALL %s
+; RUN: llc %t/a2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefix=CHECK --check-prefix=CHECK-SMALL %s
+; RUN: llc %t/b2.ll -mtriple=armv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefix=CHECK --check-prefix=CHECK-LARGE %s
+; RUN: llc %t/b2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefix=CHECK --check-prefix=CHECK-LARGE %s
----------------
You can convert `--check-prefix=CHECK --check-prefix=CHECK-SMALL` to
`--check-prefixes=CHECK,CHECK-SMALL`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112768/new/
https://reviews.llvm.org/D112768
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
