Support optnone in SCCP

[dblaikie]

Summary

Looks like any place this code checks for declarations it should also
check for the optnone attribute to avoid doing any inter-functional
optimizations based on the contents of a optnone function.

[dblaikie]

Fix think-o, this is about optnone, not about nodebug

[dblaikie]

Fix misclassification of optnone function as not executed

[fhahn]

I think this makes sense , but I don't think this behavior is clear for the current definition of optnone, which says .... with the exception of interprocedural optimization passes.. Do you think it would be worth to spell this out in a clearer way? Should all IPOs ignore optnone functions?

[probinson]

IIRC, the exception for IPOs was because we hadn't thought too much about what optnone ought to mean in that context--specifically, when two functions would interact in an IPO-ish way, but one function has optnone and the other doesn't.

I think it's fine to respecify optnone in a way that lets it factor into IPO decisions while preserving the intent of optnone (which is that we should leave them alone as much as possible). But, we should actually respecify it that way; then IPOs that don't conform become bugs/TODOs that need taking care of.

[dblaikie]

Update LangRef to clarify/improve the optnone guarantee for interprocedural optimizations.

[dblaikie]

In D100353#2688790, @fhahn wrote:

I think this makes sense , but I don't think this behavior is clear for the current definition of optnone, which says .... with the exception of interprocedural optimization passes.. Do you think it would be worth to spell this out in a clearer way? Should all IPOs ignore optnone functions?

In D100353#2692812, @probinson wrote:

IIRC, the exception for IPOs was because we hadn't thought too much about what optnone ought to mean in that context--specifically, when two functions would interact in an IPO-ish way, but one function has optnone and the other doesn't.

I think it's fine to respecify optnone in a way that lets it factor into IPO decisions while preserving the intent of optnone (which is that we should leave them alone as much as possible). But, we should actually respecify it that way; then IPOs that don't conform become bugs/TODOs that need taking care of.

Yep, makes sense to me - added LangRef update to this patch (can split it out if folks think that'd be best).

[probinson]

Probably should drop a note to llvm-dev, as the title of this review doesn't suggest it's redefining optnone.

[lebedev.ri]

I don't like this redefinition.
I would suggest to introduce noipa attribute, like i thought this patch does.

[dblaikie]

In D100353#2694589, @probinson wrote:

Probably should drop a note to llvm-dev, as the title of this review doesn't suggest it's redefining optnone.

Fair point

In D100353#2694602, @lebedev.ri wrote:

I don't like this redefinition.
I would suggest to introduce noipa attribute, like i thought this patch does.

Hrm - wasn't expecting much disagreement here, based on my understanding of optnone. My mistake - posted an llvm-dev thread to further the discussion here: https://lists.llvm.org/pipermail/llvm-dev/2021-April/149960.html

[lebedev.ri]

In D100353#2697332, @dblaikie wrote:

In D100353#2694589, @probinson wrote:

Probably should drop a note to llvm-dev, as the title of this review doesn't suggest it's redefining optnone.

Fair point

In D100353#2694602, @lebedev.ri wrote:

I don't like this redefinition.
I would suggest to introduce noipa attribute, like i thought this patch does.

Hrm - wasn't expecting much disagreement here, based on my understanding of optnone. My mistake - posted an llvm-dev thread to further the discussion here: https://lists.llvm.org/pipermail/llvm-dev/2021-April/149960.html

Thanks, replied there.

[dblaikie]

Abandoning this since the discussion ended up pointing towards a preference for a distinct noipa attribute. Then that stalled out in discussions about whether that could be implemented via isInterposable or would need a separate proprety and then revisiting/restesting every check for interposability to instead test "interposable or noipa", basically... which made me a bit exhausted to think about visiting/changing/retesting all those cases and so I haven't picked this up again. It's over in D101011 if anyone wants to weigh in there/pick it up/poke it with a stick, etc.

All rather unfortunate, I think - there's a bunch of evidence that noipa was in a bunch of ways what some people expected from optnone. Pity we didn't get that from the start/weren't willing to accept that as a bugfix, of sorts, but ah well.

llvm/docs/LangRef.rst

``null_pointer_is_valid``
    Note: Comparing address of a global variable to ``null`` may still
    evaluate to false because of a limitation in querying this attribute inside
    constant expressions.
 ``optforfuzzing``
     This attribute indicates that this function should be optimized
     for maximum fuzzing signal.
 ``optnone``
     This function attribute indicates that most optimization passes will skip
-    this function, with the exception of interprocedural optimization passes.
-    Code generation defaults to the "fast" instruction selector.
-    This attribute cannot be used together with the ``alwaysinline``
-    attribute; this attribute is also incompatible
-    with the ``minsize`` attribute and the ``optsize`` attribute.
+    this function. Interprocedural optimizations should treat this function as
+    though it were defined in an isolated module/object. Code generation
+    defaults to the "fast" instruction selector. This attribute cannot be used
+    together with the ``alwaysinline`` attribute; this attribute is also
+    incompatible with the ``minsize`` attribute and the ``optsize`` attribute.
 
     This attribute requires the ``noinline`` attribute to be specified on
     the function as well, so the function is never inlined into any caller.
     Only functions with the ``alwaysinline`` attribute are valid
     candidates for inlining into the body of this function.
 ``optsize``
     This attribute suggests that optimization passes and code generator
     passes make choices that keep the code size of this function low,

llvm/lib/Transforms/Scalar/SCCP.cpp

if (CB.getType()->isVoidTy())
     return;
 
   // Always mark struct return as overdefined.
   if (CB.getType()->isStructTy())
     return (void)markOverdefined(&CB);
 
   // Otherwise, if we have a single return value case, and if the function is
   // a declaration, maybe we can constant fold it.
-  if (F && F->isDeclaration() && canConstantFoldCallTo(&CB, F)) {
+  if (F && (F->isDeclaration() || F->hasOptNone()) &&
+      canConstantFoldCallTo(&CB, F)) {
     SmallVector<Constant *, 8> Operands;
     for (auto AI = CB.arg_begin(), E = CB.arg_end(); AI != E; ++AI) {
       if (AI->get()->getType()->isStructTy())
         return markOverdefined(&CB); // Can't handle struct args.
       ValueLatticeElement State = getValueState(*AI);
 
       if (State.isUnknownOrUndef())
         return; // Operands are not resolved yet.
if (ConstantRange::isIntrinsicSupported(II->getIntrinsicID())) {
           ConstantRange::intrinsic(II->getIntrinsicID(), OpRanges);
       return (void)mergeInValue(II, ValueLatticeElement::getRange(Result));
     }
   }
 
   // The common case is that we aren't tracking the callee, either because we
   // are not doing interprocedural analysis or the callee is indirect, or is
   // external.  Handle these cases first.
-  if (!F || F->isDeclaration())
+  if (!F || F->isDeclaration() || F->hasOptNone())
     return handleCallOverdefined(CB);
 
   // If this is a single/zero retval case, see if we're tracking the function.
   if (auto *STy = dyn_cast<StructType>(F->getReturnType())) {
     if (!MRVFunctionsTracked.count(F))
       return handleCallOverdefined(CB); // Not tracking this callee.
 
     // If we are tracking this callee, propagate the result of the function
bool llvm::runIPSCCP(
     function_ref<AnalysisResultsForFn(Function &)> getAnalysis) {
   SCCPSolver Solver(DL, GetTLI, M.getContext());
 
   // Loop over all functions, marking arguments to those with their addresses
   // taken or that are external as overdefined.
   for (Function &F : M) {
     if (F.isDeclaration())
       continue;
+    if (F.hasOptNone()) {
+      Solver.MarkBlockExecutable(&F.front());
+      continue;
+    }
 
     Solver.addAnalysis(F, getAnalysis(F));
 
     // Determine if we can track the function's return values. If so, add the
     // function to the solver's set of return-tracked functions.
     if (canTrackReturnsInterprocedurally(&F))
       Solver.AddTrackedFunction(&F);
 
bool llvm::runIPSCCP(
   }
 
   bool MadeChanges = false;
 
   // Iterate over all of the instructions in the module, replacing them with
   // constants if we have found them to be of constant values.
 
   for (Function &F : M) {
-    if (F.isDeclaration())
+    if (F.isDeclaration() || F.hasOptNone())
       continue;
 
     SmallVector<BasicBlock *, 512> BlocksToErase;
 
     if (Solver.isBlockExecutable(&F.front())) {
       bool ReplacedPointerArg = false;
       for (Argument &Arg : F.args()) {
         if (!Arg.use_empty() && tryToReplaceWithConstant(Solver, &Arg)) {

llvm/test/Transforms/SCCP/optnone.ll

+; RUN: opt < %s -ipsccp -instcombine -S | FileCheck %s
+
+attributes #0 = { noinline optnone }
+
+define i32 @callee() #0 {
+  ret i32 52
+}
+
+define i32 @caller() {
+  ; CHECK: define i32 @caller()
+  ; CHECK: ret i32 %X
+  %X = call i32 @callee()
+  ret i32 %X
+}
+