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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
