[PATCH] D112811: [ARM] implement LOAD_STACK_GUARD for remaining targets

[Ard Biesheuvel via Phabricator via llvm-commits]

ardb added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:4905-4906
+      TargetFlags |= ARMII::MO_DLLIMPORT;
+    else if (IsIndirect)
+      TargetFlags |= ARMII::MO_COFFSTUB;
+  } else if (Subtarget.isGVInGOT(GV)) {
----------------
nickdesaulniers wrote:
> Other uses of `ARMII::MO_COFFSTUB` in llvm/lib/Target/ARM/ARMISelLowering.cpp seem to check `TargetMachine::shouldAssumeDSOLocal` returns `false` (and `GlobalValue::hasDLLImportStorageClass()` returns `false`, too) before setting this flag. Should we be doing so here as well?
> 
> To get a reference to the `TargetMachine` instance, I think we can do:
> 
> ```
> MachineFunction *MF = MBB.getParent();
> ARMSubtarget &Subtarget = MF->getSubtarget<ARMSubtarget>();
> TargetMachine *TM = SubTarget.getTargetLowering()->getTargetMachine();
> ```
> 
> @peter.smith anyone with more windows on ARM experience that might be able to help triple check this? Probably @mstorsjo ?
> Other uses of `ARMII::MO_COFFSTUB` in llvm/lib/Target/ARM/ARMISelLowering.cpp seem to check `TargetMachine::shouldAssumeDSOLocal` returns `false` (and `GlobalValue::hasDLLImportStorageClass()` returns `false`, too) before setting this flag. Should we be doing so here as well?
> 

!TargetMachine::shouldAssumeDSOLocal() is implied by isGVIndirectSymbol() so we already check this.

> To get a reference to the `TargetMachine` instance, I think we can do:
> 
> ```
> MachineFunction *MF = MBB.getParent();
> ARMSubtarget &Subtarget = MF->getSubtarget<ARMSubtarget>();
> TargetMachine *TM = SubTarget.getTargetLowering()->getTargetMachine();
> ```
> 
> @peter.smith anyone with more windows on ARM experience that might be able to help triple check this? Probably @mstorsjo ?




================
Comment at: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp:250-258
 void Thumb2InstrInfo::expandLoadStackGuard(
     MachineBasicBlock::iterator MI) const {
   MachineFunction &MF = *MI->getParent()->getParent();
-  if (MF.getTarget().isPositionIndependent())
+  const GlobalValue *GV =
+      cast<GlobalValue>((*MI->memoperands_begin())->getValue());
+
+  if (MF.getSubtarget<ARMSubtarget>().isGVInGOT(GV))
----------------
nickdesaulniers wrote:
> Does `Thumb1InstrInfo::expandLoadStackGuard` need a similar change?
> 
> If so, then all the callers of `ARMBaseInstrInfo::expandLoadStackGuardBase` seem to also compute the same value of `GV` that is rematerialized in `ARMBaseInstrInfo::expandLoadStackGuardBase`. At that point, I'd say let's change the function signature of `ARMBaseInstrInfo::expandLoadStackGuardBase` to accept the `GlobalValue*` we calculated in the caller.  Basically, let's DRY up `const GlobalValue *GV = cast<GlobalValue>((*MI->memoperands_begin())->getValue());` which seems repeated in callers and callee.
> 
> Though if we don't need this change for thumb[1] then perhaps we don't need to do that (or could simply pass `nullptr` for that interface change.
No it does not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112811