nemanjai added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10702
+
+    // If SafeStack or !StackProtector, kill_canary is not supported.
+    if (MF.getFunction().hasFnAttribute(Attribute::SafeStack) ||
----------------
Again, this comment is nothing more than a re-reading of the code. As such, it 
is not useful. The comment should say what is happening and why. Something 
along the lines of:
```
// The kill_canary intrinsic only makes sense when the Stack Protector
// feature is on in the function. It can also not be used in conjunction
// with safe stack because the latter splits the stack and the canary
// value isn't used (i.e. safe stack supersedes stack protector).
// In situations where the kill_canary intrinsic is not supported,
// we simply replace uses of its chain with its input chain, causing
// the SDAG CSE to remove the node.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10720-10749
+    if (useLoadStackGuardNode()) {
+      MachineSDNode *LSG =
+          DAG.getMachineNode(PPC::LOAD_STACK_GUARD, DL, VT, Op->getOperand(0));
+      Load = SDValue(LSG, 0);
+
+      // Frame index used to determine stack guard location if
+      // LOAD_STACK_GUARD is used.
----------------
The common code should just be common. Seems like the only thing that changes 
is how we load the value. Please refactor this to something like:
```
if (useLoadStackGuardNode()) {
  ...
  Load = ... // stack guard load
} else if (Value *GV = getSDagStackGuard(*M)) {
  ...
  Load = ... // canary word global load
} else
  llvm_unreachable("Unhandled stack guard case");

Store = ... // common store
return Store; // (or you can just create the store in-line and return it 
directly)
```


================
Comment at: llvm/test/CodeGen/PowerPC/kill-canary-intrinsic.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-aix 
-ppc-vsr-nums-as-vr \
+; RUN:   -mcpu=pwr7 --ppc-asm-full-reg-names < %s | FileCheck %s 
-check-prefix=CHECK-AIX
----------------
Please add one RUN line with `-O0` to ensure that this works as expected with 
Fast ISel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129016

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to