[PATCH] D125847: LTO: Add option to initialize with opaque/non-opaque pointer types

Fri Jun 10 08:29:22 PDT 2022

MatzeB added inline comments.

================
Comment at: lld/test/ELF/lto/discard-value-names.ll:4

 ; RUN: ld.lld -shared -save-temps %t.o -o %t2.o
 ; RUN: llvm-dis < %t2.o.0.0.preopt.bc | FileCheck %s
----------------
bmahjour wrote:
> I see `--plugin-opt=opaque-pointers` is being explicitly specified for some but not all tests. I suppose the reason you explicitly specify it is to make sure the tests pass even for builds where opaque pointers are disabled. If that's the case, why aren't you doing it consistently?
It‘s not specified in any of the lld tests except the two tests for the option itself (because in lld lto its enabled by default now).

It is specified for clang (but not clang cc1) in a coupletests because the clang driver drfaults can currently be changed with a cmake flag.

================
Comment at: llvm/test/Analysis/StackSafetyAnalysis/ipa.ll:86

-; RUN: llvm-lto2 run %t.summ0.bc %t.summ1.bc -o %t.lto -stack-safety-print -stack-safety-run -save-temps -thinlto-threads 1 -O0 \
+; RUN: llvm-lto2 run -opaque-pointers=0 %t.summ0.bc %t.summ1.bc -o %t.lto -stack-safety-print -stack-safety-run -save-temps -thinlto-threads 1 -O0 \
 ; RUN:  $(cat %t.res.txt) \
----------------
bmahjour wrote:
> why does this test (and ipa-alias.ll above) need to run with opaque pointers off?
I don‘t remember this test specifically but some were tricky to update, feel free to submit a patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125847