[PATCH] D29214: [AMDGPU] Internalize non-kernel symbols

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec marked an inline comment as done.
rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:88-90
+static cl::opt<bool> WholeProgram(
+  "amdgpu-whole-program",
+  cl::desc("Enable whole program mode"),
----------------
tony-tye wrote:
> rampitec wrote:
> > rampitec wrote:
> > > arsenm wrote:
> > > > This isn't a good name (and shouldn't it default to true?). How about amdgpu-internalize-functions/
> > > It also internalizes global variables, so "functions" here are misleading.
> > > That shall not be on by default. For instance this is not suitable to build library. Instead I will submit a patch to add the option from the OpenCL itself (same applies to HCC).
> > Do you feel -amdgpu-internalize-symbols is better?
> I don't think it should be on by default at all optimization levels as it removes unused global variables and the debugger prefers that all variables remain. So perhaps -O0 (or when -g is specified with the debugger abi, which can be queried from the subtarget by debuggerSupported()) it should not run, or at least the global variable elimination part should not run.
> 
> At higher optimization running it by default seems reasonable as it drastically reduces the code object size. But only if the language does not support separate compilation (which is the case currently).
This does not run at -O0 (see Builder.OptLevel > 0 check below).
GlobalDCE pass will run in any way, I'm just running it early to save compilation time after internalize.
The main thing, it cannot be on by default. If it is default on it will immediately break all library builds, including those I'm not aware of. I see no problem to add an option to a language driver while building kernels.


Repository:
  rL LLVM

https://reviews.llvm.org/D29214