[PATCH] D52099: [PPC64] Add split-stack support.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Arch/PPC64.cpp:663
 
+bool PPC64::adjustPrologueForCrossSplitStack(uint8_t *Loc, uint8_t *End,
+                                             uint8_t StOther) const {
----------------
It perhaps needs a high-level comment describing what instruction sequence is rewritten to what instruction sequence.


================
Comment at: ELF/Arch/PPC64.cpp:665
+                                             uint8_t StOther) const {
+  // If the caller has a global entry point adjust the buffer past it to find
+  // the start of the split-stack prologue.
----------------
It's not clear to me what this code does -- is this PPC-specific thing?


================
Comment at: ELF/Arch/PPC64.cpp:676
+  // instructions it can't be a split-stack prologue.
+  if (Loc + 12 >= End) {
+    return false;
----------------
nit: remove {}


================
Comment at: ELF/Arch/PPC64.cpp:681
+  // First instruction must be `ld r0, -0x7000-64(r13)`
+  if (read32(Loc) != 0xe80d8fc0) {
+    return false;
----------------
Ditto


================
Comment at: ELF/Arch/PPC64.cpp:685
+
+  int16_t HighImmediate = 0, LowImmediate = 0;
+  // First instruction can be either an addis if the frame size is larger then
----------------
nit: I'd name HiImm and LoImm

We usually define one variable per line, so 

  int16_t HiImm = 0;
  int16_t LoImm = 0;


================
Comment at: ELF/Arch/PPC64.cpp:694
+  } else {
+    return false;
+  }
----------------
Can this happen?


================
Comment at: ELF/Arch/PPC64.cpp:703
+  } else if (SecondInstr != 0x60000000) {
+    return false;
+  }
----------------
Is this an ABI violation, or legitimate code has such code sequence in which an instruction other than addi or nop follows the first instruction?


================
Comment at: ELF/Driver.cpp:284-285
+  if (Args.hasArg(OPT_split_stack_adjust_size) && Config->EMachine != EM_PPC64)
+    error(
+        "--split-stack-adjust-size is only supported on the PowerPC64 target.");
+
----------------
This may be a silly question, but why we have `--split-stack-adjust-size` only for PPC64 and not for x86-64? If we can live without that option with x86-64, why do we need it for PPC64?


================
Comment at: ELF/Target.h:80
+  // non-split-stack callee this will return true. Otherwise returns false.
+  virtual bool needsMoreStackNonSplit(void) const;
 
----------------
This should be a boolean public member variable instead of a virtual function call for the sake of simplicity.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D52099