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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Some thoughts



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


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


================
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,
----------------
What happens for C++11+ when `-fno-finite-loops` is specified?
I.e., shouldn't this be similar to `loopMustProgress()`?


================
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:
> 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.


================
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;
----------------
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

 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96418