[AlwaysInliner] Check inliner errors even without assserts

[ellis]

Summary

When we build clang without asserts we should still check the result of
InlineFunction() to be sure there wasn't an error. Otherwise we could
incorrectly merge attributes in the next line.

This also removes a redundent call to getCaller().

[lebedev.ri]

Essentially every unchecked assertion is followed by code that is effectively UB if the assertion should have failed but didn't run.
Why is this case so special that we should special-case it?

[ellis]

In D121722#3383298, @lebedev.ri wrote:

Essentially every unchecked assertion is followed by code that is effectively UB if the assertion should have failed but didn't run.
Why is this case so special that we should special-case it?

This is a pretty easy way to prevent unexpected behavior when inlining goes wrong with noasserts builds, i.e., production.

1. Res gives a helpful error message that can be given to the user.
2. The comment for InlineFunction() claims that the program will be fine if it failed to inline, so continuing is perfectly safe.

This returns false if it is not possible to inline this call. The program is still in a well defined state if this occurs though.

[nikic]

Either inlining always succeeds and we assert, or inlining can fail and we should never assert.

I do think that it can fail, and you should be able to add a test for it. There's a bunch of checks at the start of InlineFunction that should be usable for that purpose, e.g. a personality function mismatch.

[ellis]

Add a test case for functions with the alwaysinline attribute that cannot be inlined and emit a remark instead of outputting directly to errs().

[nikic]

LGTM

[kyulee]

llvm/lib/Transforms/IPO/AlwaysInliner.cpp

81

Should it be moved down after InlineResult is successful?
This is about remarking a success case which we want only when it succeeded.

[ellis]

llvm/lib/Transforms/IPO/AlwaysInliner.cpp

81

Good catch. I've just uploaded D121946 to fix.

llvm/lib/Transforms/IPO/AlwaysInliner.cpp

if (!F.isDeclaration() && isInlineViable(F).isSuccess()) {
             [&](CallBase &CB) {
               return InlineCost::getAlways("always inline attribute");
             },
             ORE);
         assert(OIC);
         emitInlinedIntoBasedOnCost(ORE, CB->getDebugLoc(), CB->getParent(), F,
                                    *Caller, *OIC, false, DEBUG_TYPE);
 
         InlineFunctionInfo IFI(
             /*cg=*/nullptr, GetAssumptionCache, &PSI,
-            &FAM.getResult<BlockFrequencyAnalysis>(*(CB->getCaller())),
+            &FAM.getResult<BlockFrequencyAnalysis>(*Caller),
             &FAM.getResult<BlockFrequencyAnalysis>(F));
 
         InlineResult Res = InlineFunction(
             *CB, IFI, &FAM.getResult<AAManager>(F), InsertLifetime);
-        assert(Res.isSuccess() && "unexpected failure to inline");
-        (void)Res;
+        if (!Res.isSuccess()) {
+          llvm::errs() << "Unable to inline " << F.getName() << " at "
+                       << Caller->getName() << ": " << Res.getFailureReason()
+                       << "\n";
+          assert(false && "unexpected failure to inline");
+          continue;
+        }
 
         // Merge the attributes based on the inlining.
         AttributeFuncs::mergeAttributesForInlining(*Caller, F);
 
         Changed = true;
       }
 
       if (F.hasFnAttribute(Attribute::AlwaysInline)) {