clang appears not to respect __attribute__((noinline))

[fdeferriere]

Summary

Bug 26545 reports that the "noinline" attribute does not prevent the result of a function to be "inlined" into the caller.
The problem comes in fact from the Inter-Procedural Sparse Constant Propagation which propagates the return value into the caller.
This patch fixes this problem by patching the function "canTrackReturnsInterprocedurally". This function now returns false on functions with the "noinline" attribute.

Test case:
llvm/test/Transforms/IPConstantProp/noinline.ll

; RUN: opt -ipsccp -S %s | FileCheck %s

define internal fastcc i32 @v1() #0 {
entry:

ret i32 1

; No IPSCCP when noinline attribute
; CHECK: ret i32 1
}

define internal fastcc i32 @v2() {
entry:

ret i32 2

; IPSCCP on this function
; CHECK: ret i32 undef
}

define internal fastcc i32 @foo(i32 %x, i32 %y) {
entry:

%add = add nsw i32 %y, %x
%call = tail call fastcc i32 @v1()
%add1 = add nsw i32 %add, %call
%call2 = tail call fastcc i32 @v2()
%add3 = add nsw i32 %add1, %call2
ret i32 %add3

; CHECK: ret i32 %add3
}

define i32 @bar() {
entry:

%call = tail call fastcc i32 @v1()
%call1 = tail call fastcc i32 @v2()
%call2 = tail call fastcc i32 @foo(i32 %call, i32 %call1)
ret i32 %call2

; CHECK ret i32 %call2
}

attributes #0 = { noinline }

[lebedev.ri]

Test?
Also, please upload all patches with full context (-U99999)

[fdeferriere]

git diff -U99999

[fdeferriere]

Fixed typo

[chandlerc]

The intent of noinline in LLVM's IR is to block inlining, not all interprocedural optimizations. So I don't think this is actually a bug and don't think this patch should be applied.

We have an attribute to block optimizations: optnone.

[dmajor]

In D53431#1271272, @chandlerc wrote:

The intent of noinline in LLVM's IR is to block inlining, not all interprocedural optimizations. So I don't think this is actually a bug

While this position may be correct in the technical sense, I don't think it's helpful to real-world developers. To a non-compiler-engineer, what's happening is "inlining", and clang is doing an unexpected thing by dropping the function call.

When people in my organization ask me why this doesn't work, I don't want to have to explain "it's not actually inlining" nor "use optnone". It would be great if there could be a change in the behavior here (but I have no expertise on whether this specific patch is the right way to achieve it).

[chrib]

actually optnone is not well adapted here, since it demotes local optimizations on the functions, but we still want to keep scalar or target optimisations to be applied, but not interprocedural optimisations.

so maybe a new attribute , e.g ((noglobalopt) would be useful to exclude an object from all global optimizations (dfe, dve, global constant prop, ...). Do you think it is worth to make a proposal ?

[chandlerc]

In D53431#1319789, @chrib wrote:

actually optnone is not well adapted here, since it demotes local optimizations on the functions, but we still want to keep scalar or target optimisations to be applied, but not interprocedural optimisations.

so maybe a new attribute , e.g ((noglobalopt) would be useful to exclude an object from all global optimizations (dfe, dve, global constant prop, ...). Do you think it is worth to make a proposal ?

I do not think we can realistically support slicing and dicing optimizations in this way. We allow turning them off to help bisect problems and work around them, but IMO this is going too far.

lib/Analysis/ValueLatticeUtils.cpp

 #include "llvm/IR/Instructions.h"
 using namespace llvm;
 
 bool llvm::canTrackArgumentsInterprocedurally(Function *F) {
   return F->hasLocalLinkage() && !F->hasAddressTaken();
 }
 
 bool llvm::canTrackReturnsInterprocedurally(Function *F) {
-  return F->hasExactDefinition() && !F->hasFnAttribute(Attribute::Naked);
+  return F->hasExactDefinition() && !F->hasFnAttribute(Attribute::Naked)
+      && !F->hasFnAttribute(Attribute::NoInline);
 }
 
 bool llvm::canTrackGlobalVariableInterprocedurally(GlobalVariable *GV) {
   if (GV->isConstant() || !GV->hasLocalLinkage() ||
       !GV->hasDefinitiveInitializer())
     return false;
   return !any_of(GV->users(), [&](User *U) {
     if (auto *Store = dyn_cast<StoreInst>(U)) {