tentzen marked 2 inline comments as done.
tentzen added inline comments.
================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:848
// Build a switch-out if we need it:
// - if there are branch-afters threaded through the scope
----------------
rnk wrote:
> Are we sure this is the right set of predicates? It seems like we'll get the
> wrong flag in cases like:
> ```
> __try {
> mayThrow();
> goto out; // AbnormalTermination should be 1
> } __finally {
> tearDown();
> }
> out:
> return 0;
> ```
>
> I think these conditions are designed to avoid the switch when there is no
> normal fallthrough destination (!HasFallthrough). I see clang emits no switch
> for this C++ input:
>
> ```
> $ cat t.cpp
> void mayThrow();
> struct Dtor {
> ~Dtor();
> };
> int f() {
> do {
> Dtor o;
> mayThrow();
> break;
> } while (0);
> return 0;
> }
>
> $ clang t.cpp -S -emit-llvm -fno-exceptions -o - | grep switch
> # empty
> ```
Yes, this test passed.
```
store i32 3, i32* %cleanup.dest.slot, align 4
%0 = call i8* @llvm.localaddress()
%cleanup.dest = load i32, i32* %cleanup.dest.slot, align 4
%1 = icmp ne i32 %cleanup.dest, 0
%2 = zext i1 %1 to i8
call void @"?fin$0@0@Test84@@"(i8 %2, i8* %0)
%cleanup.dest1 = load i32, i32* %cleanup.dest.slot, align 4
switch i32 %cleanup.dest1, label %unreachable [
i32 3, label %out
]
```
================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:863-864
+ // pass the abnormal exit flag to Fn (SEH cleanup)
+ cleanupFlags.setHasSehAbnormalExits();
+
----------------
rnk wrote:
> This flag is set for any cleanup that needs to switch-out to multiple normal
> destinations. You have named this an "seh abnormal exit", an exit that is
> abnormal for the purposes of SEH, but I think it would be clearer to give the
> flag a more general name.
>
> Based on the existing terminology, maybe (F_)HasBranchAfter would be the
> closest to what we are looking for?
my original implementation in https://github.com/tentzen/llvm-project is still
using (F_)HasBranchAfters. it's renamed because I'm afraid it may mis-imply
it to Scope.getNumBranchAfters().
Yes, Agree, it should not be SEH specific.
how about F_HasExitSwitch?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77936/new/
https://reviews.llvm.org/D77936
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
