An error type for trace invalidation. #21

vext01 · 2019-07-12T11:02:48Z

The meat of this PR is in the first commit (see the commit message for more) and then there's a boring style change.

ltratt · 2019-07-12T12:40:14Z

tiri/src/lib.rs

+    #[ignore] // FIXME -- This will fail until we investigate why SIR is missing.
+    fn interp_simple_trace() {
+        let tracer = start_tracing(Some(TracingKind::SoftwareTracing));
+        let res = black_box(work(black_box(3), black_box(13)));


Surely there's no point in doing black_box(3) -- an integer can't realistically be optimised!

It's to prevent constant propagation into the function body. Otherwise the body may be replaced by a value.

Doesn't the outer black_box prevent that happening?

Not necessarily. The addition could still be removed. Ultimately it's the arguments to the addition operator that we must prevent the compiler from knowing ahead of time.

The outer black box is actually probably not necessary.

(It's probably sufficient to only guard only one argument with a black box, too, but having two won't hurt)

As expected, the compiler performs constant propagation:

That's very interesting! black_box does not work as I expected, which is probably my fault.

OK, so going back to this PR ;) I now think we should drop the outer black_box call since it doesn't do anything useful in this context?

Yep, I agree.

You've also highlighted that the Rust black_box docs could be improved. I might do a PR against upstream.

ltratt · 2019-07-12T12:40:26Z

yktrace/src/errors.rs

+pub enum InvalidTraceError {
+    /// There is no SIR for the DefId of a location in the trace.
+    NoSir(DefId),
+    /// Something went wrong in low-level compiler support.


This sentence doesn't parse.

I'm not sure what you mean. Seems OK here.

Does "Something went wrong in the compiler's tracing code" work better?

ltratt · 2019-07-12T12:42:36Z

One thing that might be worth thinking about is whether you want to Box your error. Basically if you don't expect errors to be returned very often, and if the size of the enum is bigger than usize (which it may well be already), then you'll reduce the size of the function's return type by Boxing errors. It's not easy to know when this is/isn't a good idea, unfortunately...

vext01 · 2019-07-12T12:52:53Z

A DefId is already bigger than a box, so it does make sense to use a box. We might aswell implement Error if we are going that route too. Then ? will work with functions returning Box<Error>.

ltratt · 2019-07-12T13:00:42Z

We might aswell implement Error if we are going that route too.

Works for me.

vext01 · 2019-07-12T13:14:57Z

then you'll reduce the size of the function's return type by Boxing errors.

Ah interesting.

Remember what Jake showed us LLVM does? If a return value is larger than a register, it'll put it on the caller's stack and pass a reference into the callee. I'd have to check if we can make that happen from Rust, but that'd be more efficient than heap allocating.

Thoughts before I investigate?

ltratt · 2019-07-12T13:22:58Z

Thoughts before I investigate?

I think it's worth investigating! I'd forgotten about that LLVM discovery.

vext01 · 2019-07-12T13:24:18Z

Successfully reproduced!

Notice how the return value is a formal parameter?

ltratt · 2019-07-12T13:25:38Z

I'd like to pretend I can make that sense of that... but it's beyond me!

vext01 · 2019-07-12T13:29:41Z

The highlighted line of the right-hand side shows the LLVM IR for the call on line 15 of the Rust program on the left.

You can see that the signature of wiggle() has been transformed from:

pub fn wiggle() -> Result<(), BiggidyError>;

To:

pub fn wiggle(err: &Result<(), BiggidyError>);

So the storage for the return value is allocated on the stack before the call, and a reference is passed into the callee. The callee then populates the pre-allocated space with the (conceptual) return value. Very cool.

ltratt · 2019-07-12T13:30:20Z

Aha, I see!

vext01 · 2019-07-12T13:32:57Z

So now the question is, do we prefer to leave the return value unboxed (thus using stack allocation) or box it and use heap allocation?

My gut says the former is more efficient because stack allocation amounts to only subtracting from the stack pointer, whereas heap allocation requires locating a spot large enough on the heap. Using the heap also contributes to memory fragmentation and I'd also guess has worse locality.

Would you agree?

ltratt · 2019-07-12T13:34:02Z

Why don't you try on your example? My guess is that boxing it still transforms things into a parameter, at which point the boxing is largely pointless (unless the struct involved is really big -- at some point the waste of stack space must be a factor, but I doubt we're at that point).

vext01 · 2019-07-12T14:04:50Z

The opposite, because the Box fits in a register:

The call is:

%3 = tail call noalias align 8 dereferenceable_or_null(24) i64* @_ZN7example6wiggle17h8f5262ea6c257490E()

Notice the i64* return type? That's our Box, and there are no formal params.

ltratt · 2019-07-12T14:22:46Z

That's probably because you've got () as the "success" part of the Result: put something in there which can't be squeezed in a pointer's spare bits, and I imagine it'll become bigger than a word.

vext01 · 2019-07-12T14:33:16Z

Yes, I'd expect the compiler to use the stack if the Ok case is larger than a register. Indeed:

call void @_ZN7example6wiggle17h4367e95236baf0f2E(%"core::result::Result<u128, alloc::boxed::Box<InvalidTraceError>>"* noalias nocapture nonnull sret dereferenceable(24) %_12)

vext01 · 2019-07-12T16:08:20Z

And what do we learn about Boxing the error type? I suppose we shouldn't as it seems LLVM does a good job of optimising it if it gets big. Agree?

ltratt · 2019-07-12T16:50:19Z

Agreed.

vext01 · 2019-07-12T18:04:03Z

The last two small changes address the outstanding comments.

ltratt · 2019-07-12T22:39:55Z

Please squash.

This makes crashes at least print some useful information instead of just telling us that a None was unwrapped. Also add a failing (but ignored for now) test which currently causes TraceInvalid::NoSir. This means we have encountered a location for which we have no SIR. A future PR will fix this, but first we will need some debugging aids before we can understand exactly what's gone wrong.

vext01 · 2019-07-13T09:59:10Z

splat

ltratt · 2019-07-13T10:58:05Z

bors r+

21: An error type for trace invalidation. r=ltratt a=vext01 The meat of this PR is in the first commit (see the commit message for more) and then there's a boring style change. Co-authored-by: Edd Barrett <[email protected]>

bors · 2019-07-13T11:14:09Z

Build succeeded

buildbot/buildbot-build-script

…ril,gnzlbg Improve the documentation for std::hint::black_box. The other day a colleague was reviewing some of my code which was using `black_box` to block constant propogation. There was a little confusion because the documentation kind of implies that `black_box` is only useful for dead code elimination, and only in benchmarking scenarios. The docs currently say: > A function that is opaque to the optimizer, to allow benchmarks to pretend to use outputs to assist in avoiding dead-code elimination. Here is our discussion, in which I show (using godbolt) that a black box can also block constant propagation: ykjit/yk#21 (comment) This change makes the docstring for `black_box` a little more general, and while we are here, I've added an example (the same one from our discussion). ![image](https://user-images.githubusercontent.com/604955/61701322-ddf1e400-ad35-11e9-878c-b5b44a20770c.png) OK to go in?

21: Start documenting our approach to PRs. r=ptersilie a=ltratt Although our approach is heavily influenced by projects like Rust, it's sufficiently elaborate that newcomers find it difficult. This commit tries to capture both the process *and* the reasons for that process. It is necessarily incomplete and imperfect: I'm sure we'll need to iterate on this for a little while before we feel fully happy with it. However, this is a start -- something is better than nothing! Co-authored-by: Laurence Tratt <[email protected]>

vext01 assigned ltratt Jul 12, 2019

ltratt reviewed Jul 12, 2019

View reviewed changes

vext01 added 2 commits July 13, 2019 10:58

Move some imports up.

e192095

vext01 force-pushed the inavlid-trace-error-type branch from 7792849 to e192095 Compare July 13, 2019 09:58

bors bot merged commit e192095 into master Jul 13, 2019

bors bot deleted the inavlid-trace-error-type branch July 13, 2019 11:14

vext01 mentioned this pull request Jul 23, 2019

Improve the documentation for std::hint::black_box. rust-lang/rust#62891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

An error type for trace invalidation. #21

An error type for trace invalidation. #21

vext01 commented Jul 12, 2019

ltratt Jul 12, 2019

vext01 Jul 12, 2019

ltratt Jul 12, 2019

vext01 Jul 12, 2019

vext01 Jul 12, 2019

vext01 Jul 12, 2019

ltratt Jul 12, 2019

vext01 Jul 12, 2019

vext01 Jul 12, 2019

ltratt Jul 12, 2019

ltratt Jul 12, 2019

vext01 Jul 12, 2019

ltratt Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019 •

edited

Loading

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019 •

edited

Loading

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 13, 2019

ltratt commented Jul 13, 2019

bors bot commented Jul 13, 2019

An error type for trace invalidation. #21

An error type for trace invalidation. #21

Conversation

vext01 commented Jul 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019 • edited Loading

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019 • edited Loading

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 12, 2019

ltratt commented Jul 12, 2019

vext01 commented Jul 13, 2019

ltratt commented Jul 13, 2019

bors bot commented Jul 13, 2019

Build succeeded

vext01 commented Jul 12, 2019 •

edited

Loading

vext01 commented Jul 12, 2019 •

edited

Loading