Skip to content
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

[MIR] implement zero-on-move #31449

Closed
wants to merge 3 commits into from
Closed

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Feb 6, 2016

This is, we think, is the last piece necessary for the fully functional implementation of dynamic dropping in MIR.

Also remove these drop-glue i8 things… They for some reason make these completely non-MIR tests
fail after dyndrop commit which has literally 0 changes around the regular old trans. Wut?
@rust-highfive
Copy link
Collaborator

r? @jroesch

(rust_highfive has picked a reviewer for you, use r? to override)

@nagisa
Copy link
Member Author

nagisa commented Feb 6, 2016

r? @nikomatsakis

NB: this is based on top of the initial dyndrop implementation PR.

@rust-highfive rust-highfive assigned nikomatsakis and unassigned jroesch Feb 6, 2016
self.store_operand(bcx, dest.llval, operand);
match operand.val {
OperandValue::Ref(v) => self.drop_fill(bcx, v, operand.ty),
OperandValue::Immediate(_) |
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’m not sure about Immediate not needing any treatment here. Immediate only happens if common::type_is_immediate returns true, but to me it is not obvious that the function will never produce true in case of type having a Drop somewhere in it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this cannot be ignored.

Also fix the tests to catch the double-drop on move.
@nagisa
Copy link
Member Author

nagisa commented Feb 6, 2016

Okay, so this won’t be as easy as I’ve initially thought. For example, call also consumes its arguments, although not through Rvalue::Use and schedules drop for them! The problem can be solved by either zeroing after call or simply not scheduling drops for the call arguments (since we know they’re moved, always).

Sadly, zeroing after call is not as trivial as it sounds: worst-case introduces 2*arg_count calls to memset(0) and it proliferates the problem described here to every branch, making implementation of MSVC SEH pretty much impossible (the hack for that particular invoke itself is fine, because we know we won’t translate invoke into a cleanup block).

As for 2nd case… The alternative is to build a new_tmp = Rvalue::Use(old_tmp) for every argument – solves the SEH things, but not the others. I’d really like to solve this at MIR-build stage, but I don’t really see the way to get rid of those drops very easily.

Any ideas, @nikomatsakis?

base::call_memset(&build::B(bcx), llptr, filling, size, align, false);
}

pub fn set_operand_dropped(&mut self,
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 also feels seriously hacky to me, but I don’t see an obvious way to dropflag-fill a OperandValue::Immediate.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use an out-of-line drop flag for these

@bors
Copy link
Contributor

bors commented Feb 9, 2016

☔ The latest upstream changes (presumably #31282) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

@nagisa

trans::mir::analyze promotes every temporary with an associated drop to an alloca, so it is safe to say that TempRef::Operand won’t ever get dropped, only consumed, and thus having it zeroed doesn’t really matter.

For example, one can do something like this consume(foo().f), which should then drop the other fields of the result. Perhaps that still works?

@nikomatsakis
Copy link
Contributor

(Did not mean to close.)

@nikomatsakis
Copy link
Contributor

I was proposing to @nagisa on IRC that we might want to restruct Call so that the return value is assigned from a special purpose rvalue in the "OK" block (this is sort of vaguely phi-like). Of course one could also consider placing the assignment on the edge somehow or using "block arguments", but it's all sort of the same thing.

Example:

Block A {
    Call(...) on-return B on-unwind C
}

Block B {
    X = CallReturn; // only legal if B has one pred that has a Call terminator
}

Block C {
}

alternatively, CallReturn could sort of be a property of the block itself, rather than a special Rvalue, but I'm not sure if that would work out cleaner overall.

Anyway, the goal here is to address the fact that the store only occurs when things succeed, and hopefully ease the translation to LLVM IR (and also ease dataflow somewhat -- @pnkfelix had to add some special cases here too).

@nikomatsakis
Copy link
Contributor

@nagisa sorry, I've kind of lost track of the state here. do you think we should land this as a "work in progress", or should we wait till we can get things working more completely?

@nagisa nagisa closed this Feb 24, 2016
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.

6 participants