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