This revision was automatically updated to reflect the committed changes.
Closed by commit rL285176: [CodeGen] Don't emit lifetime intrinsics for some
local variables (authored by vitalybuka).
Changed prior to commit:
https://reviews.llvm.org/D24693?vs=74814=75832#toc
Repository:
rL LLVM
vitalybuka marked an inline comment as done.
vitalybuka added a comment.
Slowdown from this function is below: 0.05% and it's mostly just traversing AST.
https://reviews.llvm.org/D24693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
vitalybuka marked an inline comment as done.
vitalybuka added a comment.
Please take a look. Meanwhile, I will investigate performance footprint.
In https://reviews.llvm.org/D24693#559781, @rsmith wrote:
> Is there some reasonable base set of functionality between this and
> JumpDiagnostics
vitalybuka updated this revision to Diff 74814.
vitalybuka added a comment.
- fixed comment
- added test for indirect jumps
https://reviews.llvm.org/D24693
Files:
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CMakeLists.txt
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
vitalybuka updated this revision to Diff 74813.
vitalybuka marked 5 inline comments as done.
vitalybuka added a comment.
Herald added a subscriber: modocache.
- updated comments
- indirect jumps
- optimized Detect()
https://reviews.llvm.org/D24693
Files:
lib/CodeGen/CGDecl.cpp
rsmith added a comment.
OK, so it seems like all the other approaches we discussed have problems too.
Let's move forward with this for now.
Is there some reasonable base set of functionality between this and
JumpDiagnostics that could be factored out and shared?
> VarBypassDetector.cpp:45
>
rsmith added a comment.
I'm concerned about the complexity of this approach; it's hard for me to be
confident that `BuildScopeInformation` is correct and will remain correct in
the presence of future changes to the AST, and if it's not, we'll potentially
silently miscompile.
A simpler but
vitalybuka updated this revision to Diff 72909.
vitalybuka added a comment.
rebase
https://reviews.llvm.org/D24693
Files:
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CMakeLists.txt
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
lib/CodeGen/VarBypassDetector.cpp
vitalybuka added a comment.
My assumption is that "start" makes access valid, and "end" makes access
invalid, up to the next "start".
I see no problems problems with loops and multiple regions, as soon as access
is happening between start and end. Loops always call "start" for nested alloca
on
rsmith added a comment.
Before we start with heroics here, we should consider whether the LLVM
intrinsics are actually specified the right way. The current specification does
the wrong thing for even trivial cases, such as a variable declared within a
loop, so there's some impedance mismatch
ahatanak added a comment.
In https://reviews.llvm.org/D24693#551050, @vitalybuka wrote:
> I can see how to insert starts, e.g. on every label which bypass declaration,
> but I am not sure where to put ends.
> Probably it's possible, but patch will be significantly more complicated.
> I'd
vitalybuka added a comment.
In https://reviews.llvm.org/D24693#550119, @ahatanak wrote:
> Thank you for the great example! I can now see this patch does fix
> mis-compiles.
>
> There are probably other lifetime bugs you'll see when the code being
> compiled includes gotos that jump past
ahatanak added a comment.
Thank you for the great example! I can now see this patch does fix mis-compiles.
There are probably other lifetime bugs you'll see when the code being compiled
includes gotos that jump past variable declarations, including the one here:
vitalybuka added a comment.
Miscompile.
Here assert fails without the patch.
int* p1;
int* p2;
int use2() {
assert(p1 != p2 || !"reuse");
return p1 == p2;
}
void f3(int cond) {
{
int tmp[1024];
p1 = tmp;
goto l2;
l1:
int tmp2[1024];
p2 = tmp2;
exit(use2());
ahatanak added a comment.
In https://reviews.llvm.org/D24693#549095, @majnemer wrote:
> This doesn't sound right. Given the example in the description, we are
> accessing the memory location after end has been called: this seems like a
> real miscompile. It would appear unsafe to only do
majnemer added a subscriber: majnemer.
majnemer added a comment.
In https://reviews.llvm.org/D24693#548739, @ahatanak wrote:
> Do we want to remove lifetime intrinsics when we aren't doing the
> asan-use-after-scope check? Since this isn't a mis-compile caused by
> inaccurate lifetime
vitalybuka added a comment.
Intrinsics are invalid there, code is generated in such way that variable is
being accessed after lifetime.end.
I suspect that optimizer can make invalid code because of this, but I can't
reproduce.
So I think it's safer to remove them at all and don't wait for
Intrinsics are invalid there, code is generated in such way that variable
is being accessed after lifetime.end.
I suspect that optimizer can make invalid code because of this, but I can't
reproduce.
So I think it's safer to remove them at all and don't wait for miscompile
reports.
Still if
ahatanak added a comment.
Do we want to remove lifetime intrinsics when we aren't doing the
asan-use-after-scope check? Since this isn't a mis-compile caused by inaccurate
lifetime intrinsics, I was wondering whether we should do this only when
asan-use-after-scope is on to minimize the impact
vitalybuka updated this revision to Diff 71862.
vitalybuka added a comment.
Test
https://reviews.llvm.org/D24693
Files:
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CMakeLists.txt
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
lib/CodeGen/VarBypassDetector.cpp
vitalybuka updated this revision to Diff 71858.
vitalybuka added a comment.
recovered test
https://reviews.llvm.org/D24693
Files:
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CMakeLists.txt
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
lib/CodeGen/VarBypassDetector.cpp
vitalybuka marked an inline comment as done.
vitalybuka added a comment.
The patch was split in two and I moved the test into the wrong one. I'll fix
this.
https://reviews.llvm.org/D24693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ahatanak added a comment.
It looks like the test case was removed when this patch we rebased.
https://reviews.llvm.org/D24693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ahatanak added a subscriber: ahatanak.
ahatanak added a comment.
Can you add some test cases?
https://reviews.llvm.org/D24693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
eugenis added inline comments.
Comment at: lib/CodeGen/VarBypassDetector.h:50
@@ +49,3 @@
+public:
+ void Reset(const Stmt *Body);
+
rename to smth like StartFunction()?
add some API documentation.
https://reviews.llvm.org/D24693
vitalybuka updated this revision to Diff 71721.
vitalybuka added a comment.
Rebase on https://reviews.llvm.org/D24695
https://reviews.llvm.org/D24693
Files:
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CMakeLists.txt
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
vitalybuka created this revision.
vitalybuka added a reviewer: eugenis.
vitalybuka added a subscriber: cfe-commits.
Herald added subscribers: mgorny, beanz.
Current generation of lifetime intrinsics does not handle cases like:
{
char x;
l1:
bar(, 1);
}
goto l1;
Intrinsics were be
27 matches
Mail list logo
