This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Driver] Add -ffinite-loops flags
AbandonedPublic

Authored by atmnpatel on Jan 9 2021, 5:00 PM.

Details

Summary

Following D94366, clang will strictly adheres to the language standard
when deciding whether or not to emit mustprogress attributes for
loops. This patch adds two flags: -ffinite-loops and
-fno-finite-loops that override the language standard into either
never emitting mustprogress attributes or emitting mustprogress
attributes for all loops/functions.

Diff Detail

Event Timeline

atmnpatel created this revision.Jan 9 2021, 5:00 PM
atmnpatel requested review of this revision.Jan 9 2021, 5:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2021, 5:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
atmnpatel updated this revision to Diff 315641.Jan 9 2021, 5:02 PM

Update CommandLineReference.rst to also include -fno-finite-loops

Thanks for putting up the patch!

I think the code here and D94366 could be simplified if we would introduce 2 codegen options to distinguish the differences between C and C++:

  • functions-must-progress=true/false: C++ behavior >= c++11; all threads must make forward progress, we should be able to mark all functions as mustprogress and there should be no need to mark the individual loops as mustprogress.
  • loops-must-progress=all/none/c11: all -> all loops are marked mustprogress, none -> no loops are marked, c11 -> all loops that do not match the C11 escape hatch are marked mustprogress.

Now

  • c++11 and above, functions-must-progress=true, loops-must-progress=none,
  • c11 and above functions-must-progress=false, loops-must-progress=c11,
  • -ffinite-loops -> loops-must-progress=all,
  • -fnofinite-loops -> , functions-must-progress=false, loops-must-progress=false

In CodeGen, we always add mustprogress to functions exactly if functions-must-progress=true. For loops, we could have a helper shouldMarkLoopAsMustProgress(bool C11Exception), which returns true depending on loops-must-progress and C11Exceptions, which the caller sets to true if the loop condition is one that is allowed for infinite loops in C11.

This should allow us to remove CGF::FnIsMustProgress and the code to update it; now either all functions have mustprogress or none have, depending on functions-must-progress. We still need CGLoopInfo::MustProgress, but we only need to update it depending on loops-must-progress,

I think this would overall simplify the handling in Codegen, but it's very possible that I am missing something from earlier discussions. I am also adding a few additional people who may have additional thought.

One things that's slightly odd with the proposal is that for C++, loops-must-progress would be none, which has the potential to be a bit confusing. We could make this clear in the documentation or just set it to all, although as I mentioned in the beginning, that should not really be necessary. It might be helpful/necessary if loops inlined from C++ function inside C functions should keep their mustprogress property.

clang/docs/ClangCommandLineReference.rst
2290

It looks like the options are mostly ordered. Could you move it to the right place?

xbolva00 added a comment.EditedJan 10 2021, 8:43 AM

Just note: gcc enables -ffinite-loops with opt level -O2 and higher.

Maybe we should have -ffinite-functions as well?

It is kinda strange to mix loops with functions. (-fnofiniteloops changes the progress behaviour of functions....)

Thanks for putting up the patch!

I think the code here and D94366 could be simplified if we would introduce 2 codegen options to distinguish the differences between C and C++:

  • functions-must-progress=true/false: C++ behavior >= c++11; all threads must make forward progress, we should be able to mark all functions as mustprogress and there should be no need to mark the individual loops as mustprogress.
  • loops-must-progress=all/none/c11: all -> all loops are marked mustprogress, none -> no loops are marked, c11 -> all loops that do not match the C11 escape hatch are marked mustprogress.

Now

  • c++11 and above, functions-must-progress=true, loops-must-progress=none,
  • c11 and above functions-must-progress=false, loops-must-progress=c11,
  • -ffinite-loops -> loops-must-progress=all,
  • -fnofinite-loops -> , functions-must-progress=false, loops-must-progress=false

In CodeGen, we always add mustprogress to functions exactly if functions-must-progress=true. For loops, we could have a helper shouldMarkLoopAsMustProgress(bool C11Exception), which returns true depending on loops-must-progress and C11Exceptions, which the caller sets to true if the loop condition is one that is allowed for infinite loops in C11.

This should allow us to remove CGF::FnIsMustProgress and the code to update it; now either all functions have mustprogress or none have, depending on functions-must-progress. We still need CGLoopInfo::MustProgress, but we only need to update it depending on loops-must-progress,

I think this would overall simplify the handling in Codegen, but it's very possible that I am missing something from earlier discussions. I am also adding a few additional people who may have additional thought.

One things that's slightly odd with the proposal is that for C++, loops-must-progress would be none, which has the potential to be a bit confusing. We could make this clear in the documentation or just set it to all, although as I mentioned in the beginning, that should not really be necessary. It might be helpful/necessary if loops inlined from C++ function inside C functions should keep their mustprogress property.

What happens when inlining mustprogress function into non-mustprogress function (or more generally, cross-language LTO)?
We'd need to ensure that in such cases the loops get annotated as mustprogress,
so they don't loose that status, which is why i think it may be better
to always annotate loops, even if their functions already are mustprogress

fhahn added a comment.Feb 4 2021, 10:45 AM

@atmnpatel reverse ping. Just curious if you think you'll have time to follow up on this patch soonish?

Otherwise I'm probably going to add at least the -fno-finite-loops option, because we have users which code breaks due to the mustprogress loop deletion changes (because of problems in the source code, but they need a way to disable until they can fix all sources)

@fhahn Sorry for the hold up, I don't think I'll get to this relatively soon, I'll abandon this one and D94366 to sweep the board.

atmnpatel abandoned this revision.Feb 4 2021, 10:52 AM

Wait actually, we're gonna need D94366 anyways, I'll get address @xbolva00's comments by eod.

fhahn added a comment.Feb 4 2021, 10:56 AM

Wait actually, we're gonna need D94366 anyways, I'll get address @xbolva00's comments by eod.

FWIW I think we should sort this one (D94367) first, because it may simplify D94366