This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add -ffinite-loops & -fno-finite-loops options.
ClosedPublic

Authored by fhahn on Feb 10 2021, 7:20 AM.

Details

Summary

This patch adds 2 new options to control when Clang adds mustprogress:

  1. -ffinite-loops: assume all loops are finite; mustprogress is added to all loops, regardless of the selected language standard.
  2. -fno-finite-loops: assume no loop is finite; mustprogress is not added to any loop or function. We could add mustprogress to functions without loops, but we would have to detect that in Clang, which is probably not worth it.

Diff Detail

Event Timeline

fhahn created this revision.Feb 10 2021, 7:20 AM
fhahn requested review of this revision.Feb 10 2021, 7:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 7:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
fhahn updated this revision to Diff 322679.Feb 10 2021, 7:24 AM

Add description for options.

xbolva00 added inline comments.Feb 10 2021, 7:29 AM
clang/lib/Frontend/CompilerInvocation.cpp
1923

Not needed

fhahn updated this revision to Diff 322683.Feb 10 2021, 7:31 AM

Remove accidentally copied code.

clang/lib/Frontend/CompilerInvocation.cpp
1923

Removed, thanks!

jdoerfert added inline comments.Feb 10 2021, 8:35 AM
clang/include/clang/Basic/CodeGenOptions.h
147

Can we have different names?

FiniteLoopsKind::None sounds more like what `FiniteLoopsKind::No implies.

Maybe:

enum class FiniteLoopAssumptionKind {
LANGUAGE,
ALWAYS
NEVER,
};
fhahn updated this revision to Diff 322803.Feb 10 2021, 12:57 PM

adjust naming of enum members as suggested, thanks! I think it cannot be an enum class unfortunately because the CodeGenOption machinery seems to use regular enums/unsigneds.

fhahn added inline comments.Feb 10 2021, 1:12 PM
clang/include/clang/Basic/CodeGenOptions.h
147

Updated the names, thanks! They are much better.

This revision is now accepted and ready to land.Feb 10 2021, 3:32 PM

Maybe we want these options in llvm 12 as well?

(+ Release note)

fhahn added a comment.Feb 11 2021, 3:17 AM

Maybe we want these options in llvm 12 as well?

(+ Release note)

That's a good idea. The patch currently depends on D96418. Not sure if we should pull that one on the 12.x release branch though

Maybe we want these options in llvm 12 as well?

(+ Release note)

That's a good idea. The patch currently depends on D96418. Not sure if we should pull that one on the 12.x release branch though

I think we should to keep consistency with gcc and llvm 13+.

fhahn updated this revision to Diff 323373.Feb 12 2021, 9:44 AM

Rebased the patch to apply wihtout D96418. Will land soon.

This revision was landed with ongoing or failed builds.Feb 12 2021, 11:27 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Feb 17 2021, 1:59 AM

Maybe we want these options in llvm 12 as well?

(+ Release note)

That's a good idea. The patch currently depends on D96418. Not sure if we should pull that one on the 12.x release branch though

I think we should to keep consistency with gcc and llvm 13+.

Filed https://bugs.llvm.org/show_bug.cgi?id=49219 to pick this to 12.x