[PATCH] D125278: [GlobalOpt] Relax the check for ctor priorities

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a subscriber: aeubanks.
rnk added a comment.

This looks pretty good, the uint32_t / i32 thing is a bit of a nit. Let's hear back from @nikic as well. Lastly, I think @aeubanks had to make changes to globalopt at some point, let me cc him.



================
Comment at: llvm/include/llvm/Transforms/Utils/CtorUtils.h:26
+bool optimizeGlobalCtorsList(
+    Module &M, function_ref<bool(uint64_t, Function *)> ShouldRemove);
 
----------------
The priorities are all documented to be `i32` in the LangRef, so we can shorten this here and elsewhere:
https://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable
```
%0 = type { i32, void ()*, i8* }
```


================
Comment at: llvm/lib/Transforms/Utils/CtorUtils.cpp:73
     ConstantStruct *CS = cast<ConstantStruct>(V);
-    Result.push_back(dyn_cast<Function>(CS->getOperand(1)));
+    Result.emplace_back(cast<ConstantInt>(CS->getOperand(0))->getZExtValue(),
+                        dyn_cast<Function>(CS->getOperand(1)));
----------------
The verifier checks that global_ctors uses an i32, so this can be truncated safely.


================
Comment at: llvm/lib/Transforms/Utils/CtorUtils.cpp:95
     return nullptr;
 
   for (auto &V : CA->operands()) {
----------------
If you want to be defensive, we could replicate the verifier type checks here. We should already be safe, but we could always be safer.


================
Comment at: llvm/lib/Transforms/Utils/CtorUtils.cpp:159
       continue;
     }
   }
----------------
alexander-shaposhnikov wrote:
> rnk wrote:
> > It looks to me that we attempt to fold any and all constructors, and we don't stop after we fail to evaluate one of them. I think stopping on the first one that fails would be sound. Effectively, it is "as if" the first constructors ran before the first constructor which cannot be folded.
> > 
> > This is, however, quite unsatisfying since it would prevent folding `gv3` in my linked example, but that's kind of the point.
> Oh, right. 
> To preserve the current behavior for the case when all the constructors in llvm.global_ctors have the same priority
> we can do the following:
> 1/ llvm.global_ctors "splits" into groups by priority (in a "stable" manner)
> 2/ The groups are evaluated in order (according their priorities) and  within a given group the evaluation works as before
> 3/ If we have fully evaluated & committed group_1, group_2, ... , group_n and can't fully evaluate group_{n+ 1} - then we stop (do not attempt to process group_{n + 2}, group_{n + 3} ... since the state is unknown).
> Does this sound reasonable / have I missed something ?
This sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125278