This is an archive of the discontinued LLVM Phabricator instance.

[InlineCost] Fix infinite loop in indirect call evaluation
ClosedPublic

Authored by ekatz on Oct 23 2019, 10:11 AM.

Details

Summary

Currently every time we encounter an indirect call of a known function, we try to evaluate the inline cost of that function. In case of a recursion, that evaluation never stops.

The solution I propose is to evaluate only the indirect call of the function, while any further indirect calls (of a known function) will be treated just as direct function calls, which, actually, never tries to evaluate the call.

Fix PR35469.

Diff Detail

Event Timeline

ekatz created this revision.Oct 23 2019, 10:11 AM
fedor.sergeev added inline comments.Nov 12 2019, 3:12 AM
llvm/lib/Analysis/InlineCost.cpp
1268–1269

Please, add assert here on F, e.g.:

assert(F && "call to known function expected here")
fedor.sergeev added inline comments.Nov 12 2019, 3:19 AM
llvm/lib/Analysis/InlineCost.cpp
1321

& -> && ?

fedor.sergeev added a comment.EditedNov 12 2019, 4:12 AM

Can you add a test similar to what you have here that excercises indirect call through a parameter?
Say, do

define void @func1() {
  %t = bitcast void ()* @func3 to void ()*
  tail call void @func2(void()* %t)
  ret void
}
define void @func2(void()* %f) {
  tail call void %f()
  ret void
}

and then......

define void @func6() {
  %t2 = bitcast void (void()*)* @func2 to void (void()*)* 
  %t3 = bitcast void ()* @func3 to void ()*
  tail call void %t2(void()* %t3)
  ret void
}

This results in recursive call to func2+ and your fix should handle it just right.

llvm/test/Transforms/Inline/pr35469.ll

btw, I would rather have this test named by its functionality and not by PR number.

ekatz marked 2 inline comments as done.Nov 12 2019, 6:21 AM

Can you add a test similar to what you have here that excercises indirect call through a parameter?
Say, do

define void @func1() {
  %t = bitcast void ()* @func3 to void ()*
  tail call void @func2(void()* %t)
  ret void
}
define void @func2(void()* %f) {
  tail call void %f()
  ret void
}

and then......

define void @func6() {
  %t2 = bitcast void (void()*)* @func2 to void (void()*)* 
  %t3 = bitcast void ()* @func3 to void ()*
  tail call void %t2(void()* %t3)
  ret void
}

This results in recursive call to func2+ and your fix should handle it just right.

The change you suggest, doesn't cause the crash.

llvm/lib/Analysis/InlineCost.cpp
1268–1269

Done

1321

Not a mistake, though the compiler will probably lower the later to just a simple "AND" (because they are both booleans), so I guess the later form is clearer.

ekatz updated this revision to Diff 228882.Nov 12 2019, 6:25 AM

Done requested changes.

fedor.sergeev accepted this revision.Nov 12 2019, 9:28 AM

The change you suggest, doesn't cause the crash.

would you mind to add that test here?
It should exercise a path of logic where argument comes through SimplifiedValues, so it is worth having it.

Anyway, LGTM.

This revision is now accepted and ready to land.Nov 12 2019, 9:28 AM
ekatz updated this revision to Diff 229711.Nov 17 2019, 3:13 AM

Added a test for passing a function pointer as an argument.

This revision was automatically updated to reflect the committed changes.
ekatz reopened this revision.Nov 24 2019, 2:35 AM
This revision is now accepted and ready to land.Nov 24 2019, 2:35 AM
ekatz updated this revision to Diff 230789.Nov 24 2019, 2:38 AM

Remove applying of call penalty in new places.

This broke some tests.

This revision was automatically updated to reflect the committed changes.