Redundant field read in Vec::clear()

[lnicola]

See https://play.rust-lang.org/?gist=2754f11f7236754b2121eb9e4c12aa73&version=nightly&mode=release. The length of the Vec is read before being written to.

I would have expected the peephole optimizer in LLVM to recognize that pattern. Interestingly, it's not a regression; according to https://rust.godbolt.org/, the issue was even present in rustc 1.0.

[newpavlov]

On clear you have to drop all vector elements, for which you need to read length. clear is defined as self.truncate(0). And it looks like the described behavior originates from lack of truncate inlining, because drop_in_place should be NOP for u8 and optimizer is able to properly optimize this loop:

while 0 < len {
    len -= 1;
}

Maybe we should mark truncate with #[inline]?

[lnicola]

@newpavlov No, it's not that, and the function is clearly inlined. I think truncate is written in a way that's too convoluted for the optimizer to understand. Consider the alternative here: https://play.rust-lang.org/?gist=a5a3c951dd2fada69ee4fde4eb502fc5&version=nightly&mode=release.

I'm not sure it's correct (there might be an off-by-one, and I'm not sure if there's any issue with drop_in_place panics not being handled), but the second version generates good code.

EDIT: nope, my proposal doesn't work, because it needs to handle panics.

[newpavlov]

Hm, the only difference is that in truncate compiler uses "optimization" which first compares if length is zero and only if not sets it to zero. I think it makes sense to set length only in the end of the method and add #[inline] to be safe.

[lnicola]

I think it makes sense to set length only in the end of the method

Yeah, well, that's what I hoped for, but drop_in_place might panic, and then the length will be invalid.

This might need to be fixed in LLVM.

[lnicola]

Maybe some kind of SetLenOnDrop trick can work here?