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

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:796
+  bool EmitBoolCondBranch = !C || !C->isOne();
+  bool CondIsConst = C;
   const SourceRange &R = S.getSourceRange();
----------------
lebedev.ri wrote:
> I think, if we really want to give this a name, perhaps we want something even more specific,
> perhaps `CondIsConstImm`/`CondIsConstInt`?
I updated the name to `CondIsConstInt`


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:801
+                 SourceLocToDebugLoc(R.getEnd()),
+                 loopMustProgress(CondIsConst));
 
----------------
lebedev.ri wrote:
> Now this doesn't make sense. Why must loop progress if it's condition is a constant?
> I think this should be renamed?
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?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:508-521
     if (CGM.getCodeGenOpts().getFiniteLoops() ==
         CodeGenOptions::FiniteLoopsKind::Never)
       return false;
 
+    // C++11 and later guarantees that a thread eventually will do one of the
+    // following (6.9.2.3.1 in C++11):
+    // - terminate,
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > What happens for C++11+ when `-fno-finite-loops` is specified?
> > I.e., shouldn't this be similar to `loopMustProgress()`?
> 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.
> 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`.


================
Comment at: clang/test/CodeGen/attr-mustprogress.c:8
 // RUN: %clang_cc1 -std=c11 -fno-finite-loops -triple=x86_64-unknown-linux-gnu -S -emit-llvm %s -o - | FileCheck --check-prefix=CHECK --check-prefix=C99 %s
 
 int a = 0;
----------------
lebedev.ri wrote:
> 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
> 
>  
I pushed 7ca4dd82175c to add the additional run lines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96418/new/

https://reviews.llvm.org/D96418