[PATCH] D83841: [argprom] Assessing impact of magic value MaxElements promoted on compiler performance

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO.h:34
+//see ArgumentPromotion{.cpp,.h} 
+extern unsigned MaxElemsToPromote;
+
----------------
No need to explain it is a global variable. Explain what the meaning is, e.g., what does the number indicate.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:97
+      Currently, promotion of aggregates is limited to only promote up to three elements of the aggregate "), \
+      cl::location(llvm::MaxElemsToPromote));
+
----------------
The comment is above is not necessary. The first sentence of the description is sufficient. The option should be hidden, compare other command line options please. The name of the variable is not proper.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:101
+//When flag is NOT set, the global variable defaults to 3.
+unsigned llvm::MaxElemsToPromote = 3;
+
----------------
Explain the meaning not where an extern definition is and that the value is by default three.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:1122
   return new ArgPromotion(MaxElements);
 }
 
----------------
Where is the change in the pass manager?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83841