[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.
Roman Lebedev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 16 09:21:31 PDT 2021
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
More information about the cfe-commits
mailing list