Page MenuHomePhabricator

Do not remove calls to functions that may not halt

Authored by olista01 on Sep 24 2015, 9:07 AM.



This prevents LLVM from removing calls to functions if that function may not terminate. This was previously done by several passes (including ADCE and the inliner) in cases where the callee is otherwise side-effect free.

The actual fix is in Instruction.{h,cpp}, the rest of the changes are fixing up passes that were relying on the old behaviour, in cases such as intrinsics and known library functions where it is still safe to do this.

A few of the tests had to be modified as they were checking that calls were removed when this this is not safe to do so. The only test I am concerned about is LTO/X86/parallel.ll, which appears to be the only test for parallel LTO. I've relaxed it so that it passes, but I'm not sure if it is really testing the functionality any more.

Diff Detail

Event Timeline

olista01 updated this revision to Diff 35636.Sep 24 2015, 9:07 AM
olista01 retitled this revision from to Do not remove calls to functions that may not halt.
olista01 updated this object.
olista01 set the repository for this revision to rL LLVM.
olista01 added a subscriber: llvm-commits.
vsk added a subscriber: vsk.Sep 25 2015, 7:49 AM

Thanks, some minor comments inline --


It looks like many of these entries are shared with the switch in hasOptimizedCodeGen(). Could you consider sharing some of this code?


If you're open to an alternate name for this, how about "mayHaveSideEffectsOrNotTerminate"? Then "mayHaveSideEffectsIgnoringTermination" could be changed to "mayHaveSideEffects". IMO "IgnoringX" is a little hard to read. Up to you of course :).


"might have" -> "cannot have"?


nit - extra line here

olista01 marked 2 inline comments as done.Sep 28 2015, 2:38 AM
olista01 added inline comments.

I'm not sure about this, the overlap between the two sets of functions doesn't have any meaning beyond "is side effect free and has optimised codegen", so I don't see what we gain by factoring it out.


I'm also not keen on the "IgnoringTermination" name, but I think non-termination should count as a side effect (unless ignoring it is safe for the given transformation, for example the change in SimplfyCFG.cpp), so I'd rather leave "mayHaveSideEffects" as the one that does take termination into account.

olista01 updated this revision to Diff 35848.Sep 28 2015, 2:38 AM
olista01 removed rL LLVM as the repository for this revision.

Isn't it explicitly allowed in C and C++ if the function has no other effects?

I thought the consensus was that since:

  • the only observable behaviour the compiler must preserve are (roughly) libcalls and volatile accesses,
  • it may assume progress on these fronts is made by a well-defined program

any such non-terminating but otherwise side-effect free call was UB.

Right. This patch seems contrary to the consensus of the discussion
had about this on llvm-dev.

olista01 abandoned this revision.Oct 5 2015, 2:58 AM

Ah, sorry, I hadn't found that llvm-dev thread, I'll drop this patch.

Out of interest, do you have a section reference for C99 or C++03 for non-termination without side-effects being UB? C++11 has 1.10/24, and C11 has 6.8.5/6, both of which makes this explicit, but the closest I can find in the older standards is this:

The least requirements on a conforming implementation are:

  • At sequence points, volatile objects are stable in the sense that previous evaluations are complete and subsequent evaluations have not yet occurred.
  • At program termination, all data written into files shall be identical to one of the possible results that execution of the program according to the abstract semantics would have produced.
  • The input and output dynamics of interactive devices shall take place in such a fashion that prompting messages actually appear prior to a program waiting for input. What constitutes an interactive device is implementation-defined.