Page MenuHomePhabricator

[clang] Refactor mustprogress handling, add it to all loops in c++11+.
ClosedPublic

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

Details

Summary

Currently Clang does not add mustprogress to inifinite loops with a
known constant condition, matching C11 behavior. The forward progress
guarantee in C++11 and later should allow us to add mustprogress to any
loop (http://eel.is/c++draft/intro.progress#1).

This allows us to simplify the code dealing with adding mustprogress a
bit.

Diff Detail

Event Timeline

fhahn requested review of this revision.Feb 10 2021, 7:19 AM
fhahn created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 7:19 AM
jdoerfert added inline comments.Feb 10 2021, 8:31 AM
clang/lib/CodeGen/CodeGenFunction.h
523–530

Can you modify the documentation to talk about what loops must make progress, this is the code below transcribed.

Also, I don't see how this works. Should the const-ness of the condition not be related to the standard/language to make a decision?

fhahn updated this revision to Diff 322791.Feb 10 2021, 12:34 PM

Document reasoning.

fhahn added inline comments.Feb 10 2021, 12:38 PM
clang/lib/CodeGen/CodeGenFunction.h
523–530

Can you modify the documentation to talk about what loops must make progress, this is the code below transcribed.

I added some comments. I hope they make things clearer.

Also, I don't see how this works. Should the const-ness of the condition not be related to the standard/language to make a decision?

The functionMustProgress call applies the C++ rules. So C++11 and above is already handled by the call.

Afterwards, loops with constant conditions never have to make progress (C++ before 11, any C version). Loops with non-constant conditions have to make progress in C11 and later.

Hopefully the comment makes that clearer.

Seems reasonable to me and makes things simpler. @atmnpatel Any thoughts?

clang/lib/CodeGen/CodeGenFunction.h
523–530

Now, even I understand this. Thanks :)

aaron.ballman added inline comments.Feb 11 2021, 4:07 AM
clang/lib/CodeGen/CodeGenFunction.h
523–530

Loops with non-constant conditions have to make progress in C11 and later.

Can you point me to the part of the C11 (or later) standards that state this? I don't see wording like http://eel.is/c++draft/intro.progress#1 in 5.1.2.4 in http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf

fhahn added inline comments.Feb 11 2021, 4:43 AM
clang/lib/CodeGen/CodeGenFunction.h
523–530

For C11 and later, it should be 6.8.5.6, which says:

An iteration statement may be assumed by the implementation to terminate if its controlling expression is not a constant expression,171) and none of the following operations are performed in its body, controlling expression or (in the case of a for statement) its expression-3:172)
— input/output operations
— accessing a volatile object
— synchronization or atomic operations.

It would probably be good to refer to C11 6.8.5.6 here, and the equivalent for C++ (6.9.2.3.1)?

aaron.ballman added inline comments.Feb 11 2021, 4:53 AM
clang/lib/CodeGen/CodeGenFunction.h
523–530

Ah, thank you for that -- my brain was hung up on the atomic progress guarantees not the loop ones specifically. Having a standards reference in the comments would be handy, but I don't insist.

fhahn updated this revision to Diff 323003.Feb 11 2021, 7:30 AM

Add reference to standards.

fhahn updated this revision to Diff 323568.Feb 13 2021, 11:52 AM

Rebased on top of 51bf4c0e6d4c, which added -ffinite-loops.

Some thoughts

clang/lib/CodeGen/CGStmt.cpp
796

I think, if we really want to give this a name, perhaps we want something even more specific,
perhaps CondIsConstImm/CondIsConstInt?

801

Now this doesn't make sense. Why must loop progress if it's condition is a constant?
I think this should be renamed?

clang/lib/CodeGen/CodeGenFunction.h
508–520

What happens for C++11+ when -fno-finite-loops is specified?
I.e., shouldn't this be similar to loopMustProgress()?

508–520

I may be unaware, but to me this looks wrong.
getFiniteLoops() should know the right answer always, we shouldn't need to do checks for standard versions here.

clang/test/CodeGen/attr-mustprogress.c
5

Here and for C++, i would suggest to have bullet-proof RUN line coverage:

  • -std=c99/-std=c11/-std=c18/-std=c2x
  • -std=c99/-std=c11/-std=c18/-std=c2x + -ffinite-loops
  • -std=c99/-std=c11/-std=c18/-std=c2x + -fno-finite-loops
lebedev.ri requested changes to this revision.Mar 25 2021, 1:54 PM

@fhahn Are you still at this?

This revision now requires changes to proceed.Mar 25 2021, 1:54 PM
fhahn added inline comments.Apr 7 2021, 5:53 AM
clang/lib/CodeGen/CGStmt.cpp
796

I updated the name to CondIsConstInt

801

I'm not sure I follow here. Whether the condition is constant or not is important for C11 and later. Something like while(true) {} does not have to make progress, but while (Foo) {} must make progress. I am not sure what it should be renamed to?

clang/lib/CodeGen/CodeGenFunction.h
508–520

What happens for C++11+ when -fno-finite-loops is specified?
I.e., shouldn't this be similar to loopMustProgress()?

-fno-finite-loops for effectively disables marking functions as mustprogress in C++. Otherwise mustprogress on the function may be used to infer mustprogress of loops in the function. If we could prove that there are no loops and only mustprogress functions are called, we could still add mustprogress to the function. But at this point, we can't figure that out.

I think some of the CHECK lines in the original patch were incorrect, but they should be fixed in the latest version.

getFiniteLoops() should know the right answer always, we shouldn't need to do checks for standard versions here.

-ffinite-loops as is does not impact whether we add mustprogress to functions in any way, only the C++ standard version does. (e.g. using -ffinite-loops -std=c11 won't add mustprogress to functions).

-fno-finite-loops on the other hand prevents adding mustprogress even for C++.

But this patch should not change the existing behavior, other than ignoring whether the condition is constant in C++ mode or with -ffinite-loops.

clang/test/CodeGen/attr-mustprogress.c
5

I pushed 7ca4dd82175c to add the additional run lines.

fhahn updated this revision to Diff 335786.Apr 7 2021, 5:53 AM

rebased after pre-committing additional RUN lines in 7ca4dd82175c. Also adjusts a variable name

Thank you!
Some more thoughts.

clang/lib/CodeGen/CGStmt.cpp
801

Right.
But now this says that "if the condition is a constant integer 1 or 0, the loop must make progress".
Isn't that the opposite what this should say, as per your comment?
Shouldn't it be "loop must progress unless the condition is a constant integer 1"?

clang/lib/CodeGen/CodeGenFunction.h
519–520

Since every new standard version will have to be added here,
can we invert this and just check for the know fixed set
of the versions where this doesn't apply?

525

Ah, i see.
My problem is that it's impossible to tell whether loopMustProgress is a member variable
that gets assigned CondIsConstInt value, or a "filter" function.

Maybe this (and it's function friend) should be something like checkIfLoopMustProgress or something..

539

Since every new standard version will have to be added here,
can we invert this and just check for the know fixed set
of the versions where this doesn't apply?

fhahn updated this revision to Diff 336058.Apr 8 2021, 4:06 AM

Change names of loopMustProgress to checkIfLoopMustProgress. Same for functionMustProgress.

fhahn added inline comments.Apr 8 2021, 4:12 AM
clang/lib/CodeGen/CGStmt.cpp
801

Ok, so with 'saying' do you mean loopMustProgress is a bad name? I changed the name as suggested below, but if you meant something else, please let me know.

clang/lib/CodeGen/CodeGenFunction.h
519–520

I tried, but it appears as if there's no LangOpt for C++98. (this also seems not directly related to the patch, so I guess we could do that as follow-up as well?)

525

Oh right, the name was confusing. I updated the name

539

I tried but I am not sure how to best do that, given there's no explicit C89 lang opt.

lebedev.ri accepted this revision.Apr 8 2021, 4:22 AM

Seems fine to me.
Might be good for someone else (@aaron.ballman ?) to also take a look, not sure.

Thank you.

clang/lib/CodeGen/CGStmt.cpp
801

I'm saying that i was completely misreading this code because i thought CondIsConstInt was being stored into loopMustProgress, not that loopMustProgress() was being called with CondIsConstInt arg.

This revision is now accepted and ready to land.Apr 8 2021, 4:22 AM
fhahn added a comment.Mon, Apr 19, 8:22 AM

Seems fine to me.
Might be good for someone else (@aaron.ballman ?) to also take a look, not sure.

ping. @aaron.ballman any chance you could take another quick look?

aaron.ballman accepted this revision.Mon, Apr 19, 9:38 AM

I only found one tiny nit, and otherwise it LGTM, but this is a bit out of my wheelhouse.

clang/lib/CodeGen/CodeGenFunction.h
511

Seems conceptually okay, given that we have an option to control it.

clang/lib/CodeGen/CGStmt.cpp
796

Please use an explicit != nullptr check when converting a pointer to bool outside of an obvious boolean context.

clang/lib/CodeGen/CodeGenFunction.h
519–520

The way these options work is that the later standards imply the early standards; that's why there isn't a CPlusPlus98. You just need CPlusPlus11 here.

539

Same thing as with the C++ language versions.

fhahn updated this revision to Diff 341863.Fri, Apr 30, 5:46 AM

Thanks everyone! I rebased the patch and addressed John's comments. I'll commit it shortly.

fhahn marked an inline comment as done.Fri, Apr 30, 5:49 AM
fhahn added inline comments.
clang/lib/CodeGen/CGStmt.cpp
796

Updated, thanks!

clang/lib/CodeGen/CodeGenFunction.h
519–520

Great, thanks for clarifying John! I updated the code.

This revision was landed with ongoing or failed builds.Fri, Apr 30, 6:24 AM
This revision was automatically updated to reflect the committed changes.