mehdi_amini added inline comments.
================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:530
+ // Don't need to mark Objective-C methods or blocks since the undefined
+ // behaviour optimization isn't used for them.
+}
----------------
Quuxplusone wrote:
> This seems like a trap waiting to spring on someone, unless there's a
> technical reason that methods and blocks cannot possibly use the same
> optimization paths as regular functions. ("Nobody's gotten around to
> implementing it yet" is the most obvious nontechnical reason for the current
> difference.) Either way, I'd expect this patch to include test cases for both
> methods and blocks, to verify that the behavior you expect is actually the
> behavior that happens. Basically, it ought to have a regression test
> targeting the regression that I'm predicting is going to spring on someone as
> soon as they implement optimizations for methods and blocks.
>
> Also, one dumb question: what about C++ lambdas? are they FunctionDecls too?
> test case?
> This seems like a trap waiting to spring on someone, unless there's a
> technical reason that methods and blocks cannot possibly use the same
> optimization paths as regular functions.
The optimization path in LLVM is the same. I think the difference lies in clang
IRGen: there is no "unreachable" generated for these so the optimizer can't be
aggressive. So this patch is not changing anything for Objective-C methods and
blocks, and I expect that we *already* have a test that covers this behavior
(if not we should add one).
================
Comment at: test/CodeGenCXX/return.cpp:21
+ // CHECK-NOSTRICT-NEXT: load
+ // CHECK-NOSTRICT-NEXT: ret i32
+ // CHECK-NOSTRICT-NEXT: }
----------------
Quuxplusone wrote:
> Can you explain why a load from an uninitialized stack location would be
> *better* than a trap and/or `unreachable`? IIUC this is basically what Mehdi
> is asking: i.e., can you explain the rationale for this patch, because I
> don't get it either. It *seems* like a strict regression in code quality.
There is a difference. LLVM will optimize:
```
define i32 @foo() {
%1 = alloca i32, align 4
%2 = load i32, i32* %1, align 4
ret i32 %2
}
```
to:
```
define i32 @foo() {
ret i32 undef
}
```
Which means "return an undefined value" (basically any valide bit-pattern for
an i32). This is not undefined behavior if the caller does not use the value
with a side-effect.
However with:
```
define i32 @foo() {
unreachable
}
```
Just calling `foo()` is undefined behavior, even if the returned value isn't
used.
So what this patch does is actually making the compiled-code `safer` by
inhibiting some optimizations based on this UB.
================
Comment at: test/CodeGenCXX/return.cpp:35
+// CHECK-NOSTRICT: @_Z21alwaysOptimizedReturn4Enum
+// CHECK-NOSTRICT-OPT: @_Z21alwaysOptimizedReturn4Enum
+int alwaysOptimizedReturn(Enum e) {
----------------
This should be `-LABEL` check lines. And instead of repeating 4 times the same
check, you could add a common prefix.
================
Comment at: test/CodeGenCXX/return.cpp:47
+ // CHECK-NOSTRICT-OPT-NEXT: zext
+ // CHECK-NOSTRICT-OPT-NEXT: ret i32
+}
----------------
Document what's going on in the tests please.
Repository:
rL LLVM
https://reviews.llvm.org/D27163
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
