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