Skip to content

Fix lifetime encoding to match the new langref wording #686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 7, 2021

Conversation

aqjune
Copy link
Member

@aqjune aqjune commented Mar 5, 2021

Fixes #433

Conservatively, this patch still raises unsupported feature error if lifetime's pointer has alloca/non-alloca objects mixed.

@aqjune aqjune changed the title [WIP] fix lifetime to fill poison if ptr isn't alloca or offset is nonzero [WIP] fix lifetime encoding to match the new langref wording Mar 5, 2021
@RalfJung
Copy link

RalfJung commented Mar 5, 2021

So how does this handle the acausal nature of alloca+lifetime.start, where the behavior of alloca depends on which operations are going to be performed in the future of the execution?

@aqjune
Copy link
Member Author

aqjune commented Mar 6, 2021

Hmm, I think we should raise unsupported instruction if the program contains a 'bad-looking' lifetimes. A lifetime call is good-looking if it exactly determines which alloca must be dead in the beginning.
I just realized that Alive2 was already raising the unsupported error for such lifetime intrinsic in the past, so it is consistent with Alive2's current policy as well.


Value& getSize() const { return *size; }
Value* getMul() const { return mul; }
bool initDead() const { return initially_dead; }
void markAsInitiallyDead() { initially_dead = true; }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously llvm2alive.cpp's lifetime.star/end encoding was splitted into two functions: hasLifetimeStart (which checks AllocaInst's uses) and visitIntrinsicInst (which adds StartLifetime instance)
I removed hasLifetimeStart, and made visitIntrinsicInst to update this initially-dead flag if lifetime.start uses the alloca.
This markAsInitiallyDead function is added to implement it.

Pointer p(*this, bid, expr::mkUInt(0, bits_for_offset));
expr blksz = p.blockSize();
memset(p.release(), IntType("i8", 8).getDummyValue(false),
move(blksz), bits_byte / 8, {}, false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this encoding can be optimized, but fillPoison() is called only when there are multiple lifetime.start/ends, or they are taking non-stack-allocated objects, etc. These cases are rare.

hasLifetimeStart(i, visited)));
auto alloc = make_unique<Alloc>(*ty, value_name(i), *size, mul,
pref_alignment(i, i.getAllocatedType()));
allocs.emplace(&i, make_pair(alloc.get(), /*has lifetime.start?*/ false));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag will be turned on by getLifetimeKind if it finds lifetime.start with a pointer to this alloca.

@@ -12,3 +23,4 @@ define void @f() {
unreachable
}

; ERROR: Source is more defined than target
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a non-functional change; just merges lifetime-noub.src.ll/.tgt.ll into lifetime-noub.srctgt.ll

@aqjune aqjune changed the title [WIP] fix lifetime encoding to match the new langref wording Fix lifetime encoding to match the new langref wording Mar 7, 2021
@aqjune
Copy link
Member Author

aqjune commented Mar 7, 2021

Now is the time to run LLVM unit tests.. It takes about 3 hrs, I'll leave the result here when it's done.

@aqjune
Copy link
Member Author

aqjune commented Mar 7, 2021

No regression from LLVM unit tests

@nunoplopes nunoplopes merged commit c8cf134 into AliveToolkit:master Mar 7, 2021
@aqjune aqjune deleted the lifetime branch February 16, 2023 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GVN transformation regarding lifetime.start
3 participants