[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/ArgumentPromotion.h:19
+//This global value can be changed via user input to set the maximum number of members that can be promoted with an aggregate.
+static unsigned GL_NUMPROMARGS = 3;
 class TargetTransformInfo;
----------------
The comment describes something unrelated.
Please rename the variable according to the naming convention.
A static variable definition in a header is problematic if the header is included more than once. Please use an extern variable declaration here and put the definition in the cpp file.



================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:94
+//opt flag to capture user supplied value for GV: GL_NUMPROMARGS
+cl::opt<unsigned, true> argmax("numargspromoted", cl::desc("Specify number of args to promote"), cl::value_desc("numargs"), cl::location(llvm::GL_NUMPROMARGS));
+
----------------
use a more expressive command line option name, e.g., `argpromotion-max-argument-increase-per-promotion`. Please update the description to describe what this actually controls. No need for value_desc.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:1115
+  return new ArgPromotion(GL_NUMPROMARGS);
 }
 
----------------
Please do not simply ignore the argument `MaxElements` that is passed here by the user of this function. If you want to modify the default for the old pass manager, which is using this function, please update the call site where there is a 3.


================
Comment at: llvm/test/Transforms/ArgumentPromotion/fourmember-struct-promote-DEFAULT.ll:91
+!1 = !{i32 7, !"PIC Level", i32 2}
+!2 = !{!"clang version 11.0.0 (https://github.com/teamiceberg/llvm-project.git 1fb9041df0182c3e582bfcb5875e6af743eab0a1)"}
----------------
No need for the metadata and attributes at the end of this file, please remove.


================
Comment at: llvm/test/Transforms/ArgumentPromotion/fourmember-struct-promote-INTMAX.ll:1
+; RUN: opt < %s -basic-aa -mem2reg -argpromotion -mem2reg -numargspromoted=2147483647 -S | FileCheck %s
+
----------------
Please merge all three test files into one with 3 RUN lines and different check labels.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83841