[PATCH] D136484: [CodeGen] Improve large stack frame diagnostic

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

Looks helpful!  Just some pedantic non-blocker comments about style.



================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:287
   }
+  auto UnsafeStackSize = MFI.getUnsafeStackSize();
   if (MF.getFunction().hasFnAttribute(Attribute::SafeStack)) {
----------------
uint64_t

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:290
+    StackSize += UnsafeStackSize;
   }
   if (StackSize > Threshold) {
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:294
     F.getContext().diagnose(DiagStackSize);
+    auto SpillSize = 0;
+    for (int Idx = MFI.getObjectIndexBegin(), End = MFI.getObjectIndexEnd();
----------------
Please don't use `auto`.
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
`int64_t` to match the addition of `MFI::getObjectSize`.


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:295-300
+    for (int Idx = MFI.getObjectIndexBegin(), End = MFI.getObjectIndexEnd();
+         Idx != End; ++Idx) {
+      if (MFI.isSpillSlotObjectIndex(Idx)) {
+        SpillSize += MFI.getObjectSize(Idx);
+      }
+    }
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:302
+
+    auto &OS = llvm::dbgs();
+    float SpillPct = SpillSize / (float)StackSize;
----------------
Don't need `llvm::` namespace prefix.

I think it's simpler to just use `dbgs()` everywhere?


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:304
+    float SpillPct = SpillSize / (float)StackSize;
+    float VarPct = 1 - SpillPct;
+    float UnsafePct = UnsafeStackSize / (float)StackSize;
----------------
`1.0f`


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:305
+    float VarPct = 1 - SpillPct;
+    float UnsafePct = UnsafeStackSize / (float)StackSize;
+    auto VariableSize = StackSize - SpillSize;
----------------
Sink computing this into the condition below that is it's only consumer?


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:306
+    float UnsafePct = UnsafeStackSize / (float)StackSize;
+    auto VariableSize = StackSize - SpillSize;
+    OS << formatv("{0}/{1} ({3:P}) spills, {2}/{1} ({4:P}) variables",
----------------
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136484