ardb added inline comments.
================
Comment at: llvm/lib/Target/ARM/ARMInstrInfo.cpp:102
+ if (M.getStackProtectorGuard() == "tls") {
+ expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+ return;
----------------
nickdesaulniers wrote:
> 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.
> 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.
I could not figure out how to check this at command line parsing time. If we
check it later, we basically crash the compiler, right? Not very elegant ...
================
Comment at: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp:250
void Thumb2InstrInfo::expandLoadStackGuard(
MachineBasicBlock::iterator MI) const {
----------------
nickdesaulniers wrote:
> 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.
> Let's add a test for such case.
Same problem: I could not figure out how to decide early enough whether we are
targetting Thumb1 or Thumb2/ARM
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
