Start converting MIR to SSA TIR. #10

vext01 · 2019-03-15T14:37:29Z

This change:

Starts adding support for statements to TIR.
Only simple assignments are translated. Everything else is made into
an "unimplemented" statement. The plan is to keep going until the BF
interpreter has none of these.
Does the first part of the SSA conversion, namely:
- Computes the dominance frontier.
- Inserts PHI nodes in the right place and for the correct TIR variables.

So what we get is a CFG with PHIs, but which is not yet in SSA. The
remaining step is to walk the dominator tree and assign each definition
of a variable with a new SSA variable name. That's yet another algorithm
from the Appel book. To keep PRs manageable, I think we should do that
as a separate PR.

Manual inspection of CFG graphs with assignments and PHIs shown
look good. Here's a couple of examples:

example 1

example 2

I'd like to add some tests to this code to try and weed out some early
bugs (I'm sure there will be some). This is likely to take me a little
while as I want to inspect how upstream does this kind of testing. I
want to avoid manually constructing MIRs and TIRs by hand if I can help
it.

Question: Currently PHI nodes are of arbitrary arity to make them easier
to read during development. We talked about having only binary PHIs. Is
that still what we want?

[There is a local ykpack Cargo path until we decide what to do with
PHIs arity]

Cheers.

vext01 · 2019-03-15T14:38:32Z

src/librustc_yk_sections/Cargo.toml

@@ -12,5 +12,8 @@ test = false
 [dependencies]
 rustc = { path = "../librustc" }
 rustc_yk_link = { path = "../librustc_yk_link" }
-ykpack = { git = "https://github.com/softdevteam/ykpack" }
+#ykpack = { git = "https://github.com/softdevteam/ykpack" }


There is a local ykpack Cargo path until we decide what to do with
PHIs arity

So ignore this for now. I'll have a PR for ykpack once we've talked about PHI arity.

vext01 · 2019-03-15T14:41:53Z

By the way, the variable names in the alrogithmy parts of the code are named the same as in the book. So if you are wondering why the names are so obviously not "edd-like", that's why :)

ltratt · 2019-03-15T14:45:40Z

I have no strong opinions on arity: clearly binary phi nodes are expressive enough to implement everything else in, and for the (probably) common case they're the most efficient way of doing it. I'd probably be inclined to start with binary phi nodes and change later if that turns out to be unduly awkward.

ltratt · 2019-03-15T14:46:59Z

src/librustc_yk_sections/mir_cfg.rs

+    tcx: &'a TyCtxt<'a, 'tcx, 'gcx>,
+    /// The definition sites of TIR variable in terms of basic blocks.
+    def_sites: RefCell<Vec<Vec<BasicBlock>>>,
+    /// The next TIR variable index to be issued.


Huh? Is this a fancy-pants yet vague way of saying "This is a monotonically increasing integer which is used to give Tir nodes a unique ID"?

Yes, but it's current value is the one which will be given out next.

[I'm likely to abstract this mechanism in the next PR, as the last stage requires us to re-number all of our variables in a similar fashion. The reason there is a mapping in the conversion is because I don't think we will get far with a one-to-one mapping from MIR to TIR variables, unless we also model projections in the same way as Rust does.]

It doesn't matter if this is the current or next index: it's the monotonically increasing bit which is important.

ltratt · 2019-03-15T14:48:05Z

src/librustc_yk_sections/mir_cfg.rs

+        }
+    }
+
+    /// Issues a fresh TIR variable index.


Something like "Returns a guaranteed unique TIR variable index"? "Fresh" isn't exactly wrong, but I've only ever seen it used to refer to variable names.

ltratt · 2019-03-15T14:48:34Z

src/librustc_yk_sections/mir_cfg.rs

+        var_idx
+    }
+
+    /// Get the TIR variable corresponding with the MIR variable, creating a fresh one if needed.


Should this function be called mir_var_to_tir or similar?

It could be... I don't think it improves the meaning much though!

"give me an index for this MIR variable, make a new index if we havne't seen it yet".

In general, Rust has a dislike of get functions, and I've come to think this is generally a good idea.

how about just tir_var() then?

ltratt · 2019-03-15T14:51:32Z

src/librustc_yk_sections/mir_cfg.rs

+        let mut var_map = self.var_map.borrow_mut();
+
+        // Resize the backing Vec if necessary.
+        if var_map.len() <= usize::try_from(local_u32).unwrap() {


I would prefer to do this as:

debug_assert!(usize::max_value >= u32::max_value()); if var_map.len <= usize::from(local.as_u32()) { ... }

because the try_from is never really necessary (we're never going to run on a 16 bit machine) so we might as well avoid the performance hit.

Ah, I was meant to ask about try_from in the PR description.

We have to convert in both directions. From u32 to usize and back.

The u32 to usize direction can be solved as you say. What about the inverse?

In general usize to u32 is clearly unsafe on a 64 bit machine. But if we know that we only ever wrote out 32 bit numbers, the inverse is safe. It's fine to do something like this:

// Since we only ever issue numbers which fit into 32 bits, this conversion // is guaranteed to be safe. let x: usize = ...; let y: u32: x as u32;

But each case must be clearly documented.

OK, seems fine by me.

[I think Rust really makes this harder than it needs to be :( ]

Don't start me on my Rust-integer-conversions-aren't-great rant!

By the way, in cases like:

debug_assert!(size_of::<usize>() >= size_of::<T>());

Might be worth double checking T must be unsigned?

Yes, that is enforced through the num_traits crate.

Cool, one more thing, you suggested:

if var_map.len <= usize::from(local.as_u32()) { ... }

But I think it'll need to be as as conversion, due to:

79 | if var_map.len() <= usize::from(local_u32).unwrap() { | ^^^^^^^^^^^ the trait `std::convert::From<u32>` is not implemented for `usize`

Is that OK?

The assertion guarantees that the as is safe, right? But, in general, we should add a comment at each as site documenting why it's safe (e.g. "this is guaranteed by the execution in f, which must have been called before this function").

Agreed on both counts.

ltratt · 2019-03-15T14:51:48Z

src/librustc_yk_sections/mir_cfg.rs

+
+        // Resize the backing Vec if necessary.
+        if var_map.len() <= usize::try_from(local_u32).unwrap() {
+            var_map.resize(usize::try_from(local_u32 + 1).unwrap(), None);


The + here needs to be checked_add because we could overflow.

ltratt · 2019-03-15T14:52:08Z

src/librustc_yk_sections/mir_cfg.rs

+            var_map.resize(usize::try_from(local_u32 + 1).unwrap(), None);
+        }
+
+        let v = match var_map[local] {


This looks like unwrap_or_else in disguise?

i.e. this could be var_map[local].unwrap_or_else(|| ...) without the let v = or match etc.

I'll try. This part was hard to get pass the borrow checker.

ltratt · 2019-03-15T14:53:06Z

src/librustc_yk_sections/mir_cfg.rs

+    fn push_def_site(&self, bb: BasicBlock, var: TirVarIndex) {
+        let mut sites = self.def_sites.borrow_mut();
+        if sites.len() <= usize::try_from(var).unwrap() {
+            sites.resize((var + 1) as usize, Vec::new());


+ probably needs to be checked_add.

ltratt · 2019-03-15T14:53:47Z

src/librustc_yk_sections/mir_cfg.rs

+
+            // FIXME - rename variables with fresh SSA names.
+
+            // Put the finalised TIR to disk.


This comment is redundant.

ltratt · 2019-03-15T14:54:08Z

src/librustc_yk_sections/mir_cfg.rs

@@ -64,43 +150,192 @@ pub fn generate_tir<'a, 'tcx, 'gcx>(
    Ok(ret)
 }

+/// Lazy computation of dominance frontiers.
+///
+/// See Page 404 of the 2nd edition of the Appel book mention above.


"mentioned". Can also simplify "Page 404" to "p404" if you want.

ltratt · 2019-03-15T14:54:42Z

src/librustc_yk_sections/mir_cfg.rs

+impl<'a, 'tcx> DominatorFrontiers<'a, 'tcx> {
+    fn new(mir: &'a Mir<'tcx>) -> Self {
+        let num_blks = mir.basic_blocks().len();
+        let mut df = IndexVec::with_capacity(num_blks);


Does IndexVec have a from_elem function? If so, it'll probably execute faster than this + the for loop.

Yes, it does. Much neater.

ltratt · 2019-03-15T14:56:02Z

src/librustc_yk_sections/mir_cfg.rs

+                let b = BasicBlock::from_u32(b.as_u32());
+                if self.doms.is_dominated_by(b, n) && b != n {
+                    children.push(b);
+                    // Force the frontier of `b` into `self.df`. Doing this here avoids a


Not sure I understand the comment or why get does anything.

Oh, this part of the code was horrendous to get into rust.

TLDR: get() has side effects.

Long answer: if were were literally translating the algorithm from the book, we'd have done the get in the next chunk of the algorithm, but it's impossible to get past the borrow checker. Get mutates an internal cache inside the dominator frontier struct. By mutating now, we can ensure the entry exists and in the next chunk do a regular index operation without any mutation.

The cache exists because the dominator frontier algorithm is like a big pot of simultaneous equations and without a cache we'd compute the same things over and over. It's a bit like memoising fib.

Hard to explain. Does it make sense?

If get has side effects, it seems like a weird name for that function!

We can call it whatever we like. In the book it's a regular hash-table operation.

How about frontier() because that's what it returns.

That seems like a better name.

ltratt · 2019-03-15T14:57:02Z

src/librustc_yk_sections/mir_cfg.rs

+            a_orig.resize(blocks.len(), Vec::default());
+            for (a, def_blks) in self.def_sites.iter().enumerate() {
+                for bb in def_blks {
+                    a_orig[*bb].push(u32::try_from(a).unwrap())


What type does a have? Does this need to be try_from?

A is a usize here as it comes from enumerate.

ltratt · 2019-03-15T14:57:31Z

src/librustc_yk_sections/mir_cfg.rs

+        for (a, def_sites) in self.def_sites.iter().enumerate() {
+            let mut w = def_sites.clone();
+            while w.len() > 0 {
+                let n = w.pop().unwrap();


The intermediate variable seems of little use?

Solely to pair with the original algorithm:

I think that text needs to be bigger ;)

Anyway, fair enough.

ltratt · 2019-03-15T14:58:06Z

src/librustc_yk_sections/mir_cfg.rs

+        let mut a_phi: Vec<Vec<TirVarIndex>> = Vec::with_capacity(num_blks);
+        a_phi.resize(num_blks, Vec::new());
+        for (a, def_sites) in self.def_sites.iter().enumerate() {
+            let mut w = def_sites.clone();


Hang on, below we push into w, which is a clone of def_sites, so I think this loop has no effect on def_sites or ...?

Hrm. Interesting. At the time of writing I think that was intentional, but now I look again, the def site for any given variable is not used subsequently, so I think we can operate on the list in-place.

It could have had to do with the borrow checker ;) I'll see what I can do!

ltratt · 2019-03-15T14:59:04Z

OK, so we definitely need tests for this, even if they're very basic ones. In my experience, these sorts of algorithms are too easy to get subtly wrong (or, at least, I get them wrong often!).

vext01 · 2019-03-15T15:20:12Z

OK, so we definitely need tests for this

Yes, totally agreed!

Especially since it worked too quickly. 2nd or 3rd try after getting it through the borrow checker! I fear things which work close to first time...

vext01 · 2019-03-15T15:22:46Z

I'd probably be inclined to start with binary phi nodes and change later if that turns out to be unduly awkward.

It's actually easier for me to understand n-ary PHI nodes.

Imagine what this would look like once you've unrolled all of the n-ary PHIs:

So unless you object, I'd rather start with n-ary and change to binary if/when we have a good reason to do so. Would that be ok?

ltratt · 2019-03-15T15:25:12Z

What do other people do?

vext01 · 2019-03-15T15:31:38Z

It looks like LLVM uses n-ary PHI nodes:
https://llvm.org/docs/LangRef.html#phi-instruction

ltratt · 2019-03-15T15:33:04Z

OK.

vext01 · 2019-03-18T17:22:55Z

With regards to tests, and further to our MM convo... (cc @jacob-hughes)

Having read how the black box tests work in rustc, I think what we can do is extend the compiletest tool to add our own suite kind.

Our suite would be quite similar to the MirOpt test suite:
https://github.com/rust-lang/rust/blob/0f88167f89fffe321590c5148f21b7d51d44388d/src/tools/compiletest/src/runtest.rs#L2864-L2877

So you can imagine that we would have a run_tir_test() which dumps the TIR (instead of the MIR) of the input and then does matching against it.

I think by doing it like this we can borrow a lot of existing infra. But we would need to write our own TIR -> text dumper.

I'm toying with the idea of doing tests as a fresh PR, as this one is already quite big.

Would that be OK?

ltratt · 2019-03-18T22:38:40Z

src/librustc_yk_sections/mir_cfg.rs

-            var_map.resize(usize::try_from(local_u32 + 1).unwrap(), None);
+        // Vector indices are `usize`, but variable indices are `u32`, so converting from a
+        // variable index to a vector index is always safe if a `usize` can express all `u32`s.
+        debug_assert!(size_of::<usize>() >= size_of::<u32>());


This should be an assert. If it's true, the optimiser will remove it, but it means that even people on release mode will find out if (somehow) our code will break.

Good point. Shall I change all of the numeric range debug assertions to full on assertions?

ltratt · 2019-03-18T22:38:57Z

src/librustc_yk_sections/mir_cfg.rs

-
-        v
+        var_map[local].unwrap_or_else(|| {
+            // Make a new variable.


I'm not sure this comment is really needed?

ltratt · 2019-03-18T22:40:00Z

src/librustc_yk_sections/mir_cfg.rs

@@ -98,10 +96,16 @@ impl<'a, 'tcx, 'gcx> ConvCx<'a, 'tcx, 'gcx> {
    /// Add `bb` as a definition site of the TIR variable `var`.
    fn push_def_site(&self, bb: BasicBlock, var: TirVarIndex) {
        let mut sites = self.def_sites.borrow_mut();
-        if sites.len() <= usize::try_from(var).unwrap() {
-            sites.resize((var + 1) as usize, Vec::new());
+        // Vector indices are `usize`, but variable indices are `u32`, so converting from a


I would probably delete this comment, and then put (above the let var_usize line) a short comment along the lines of "var is u32 which the earlier assert guarantees can fit into a usize".

Delete the assertion too, I think?

Only if the assertion was 100% guaranteed to have been made my some other bit of code (and, if so, we should document it).

The assertion is identical to the earlier one you referenced.

But let me suggest we leave it in, since it protects from the other being removed later, and we've established that these assertions are pretty much free. What do you think?

Yes, I know it's identical, but is it guaranteed to have been called before this code is executed?

Ah sorry. I see.

Yes, the variable will have to have been created prior to it's def site being pushed.

vext01 · 2019-03-25T11:34:58Z

Hey Laurie. That last commit is a rough attempt at abstracting so that the algorithm still looks like the one in the Appel book, while using a single bit vector for each mapping.

One thing that crosses my mind. Currently VobMapSet (terrible name, needs a better one) works only on usizes meaning the consumer has to do casting. We could try to make it generic over the TryInto<usize> (using PhantomData since there are not really any numbers stored inside). Then the consumer wouldn't need to care about casting.

My questions to you now:

Do you like where this is going?
Shall we make the data structure generic?

If you do like the direction, depending on the answer to the latter question, I'd still need to go back and check the casting logic and re-comment those.

Alternatively, we can cut our losses and go back to vectors of bitsets 🔪 It wouldn't hugely matter, since this isn't a performance critical section.

Cheers

vext01 · 2019-03-25T11:39:03Z

[needs final testing btw, building now]

ltratt · 2019-03-25T11:43:12Z

I'm a bit surprised by all the complexity: don't we know exactly how big the bitvec needs to be when we create it? If so, why have we got all this resizing for? Maybe I'm missing out on something!

[I also thought you were going to use BitVec -- not that I mind you using Vob! -- but because I thought BitVec was already part of rustc.]

vext01 · 2019-03-25T11:49:36Z

don't we know exactly how big the bitvec needs to be when we create it?

In one case we do not. I've made the assumption that it may not be the case that there's one TIR variable per MIR variable. I think that would be too restrictive. Hence the resizing when we are building up our definition sites mapping during conversion to a pack.

I also thought you were going to use BitVec

I can't find a structure of this (or similar) name in the compiler or std lib.

ltratt · 2019-03-25T11:55:00Z

It might be called bit-vec or bit_vec.

don't we know exactly how big the bitvec needs to be when we create it?
In one case we do not.

Which case?

vext01 · 2019-03-25T12:00:53Z

Which case?

Like I said above. It's when we are building up our definition sites mapping. We don't know how many TIR variable there will be.

It might be called bit-vec or bit_vec.

Alas not.

I found this comment in the compiler:

// Variants is a BitVec of indexes into adt_def.variants.

But there is no further references to BitVec so I think it must have been removed.

ltratt · 2019-03-25T12:03:32Z

Like I said above. It's when we are building up our definition sites mapping.

Can you point it out to me? Maybe I'm just being dense!

Weirdly, I can see bit-vec in the Cargo.lock:

$ rg bit-vec
Cargo.lock
119: "bit-vec 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)",
123:name = "bit-vec"
3991:"checksum bit-vec 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "4440d5cb623bb7390ae27fec0bb6c61111969860f8e3ae198bfa0663645e67cf"

but can't see it (or bit_vec) being used?!

vext01 · 2019-03-25T12:04:42Z

Weirdly, I can see bit-vec in the Cargo.lock

That'll be a transitive dependency then?

vext01 · 2019-03-25T12:05:33Z

src/librustc_yk_sections/mir_cfg.rs

+    /// The compiler's god struct. Needed for queries etc.
+    tcx: &'a TyCtxt<'a, 'tcx, 'gcx>,
+    /// Maps TIR variables to their definition sites.
+    def_sites: RefCell<VobMapSet>,


This is the mapping we can't know the size of ahead of time.

vext01 · 2019-03-25T12:05:45Z

src/librustc_yk_sections/mir_cfg.rs

+
+        Self {
+            tcx,
+            def_sites: RefCell::new(VobMapSet::new(0, num_blks)),


We instantiate it empty here.

vext01 · 2019-03-25T12:06:04Z

src/librustc_yk_sections/mir_cfg.rs

+        // This conversion is safe because `var` was generated by `tir_var()` which guarantees that
+        // a `u32` can fit in a `usize`.
+        let var_usize = var as usize;
+        sites.insert_maybe_resize(var_usize, bb.as_usize());


And we insert here with a resize if necessary.

ltratt · 2019-03-25T13:09:21Z

src/librustc_data_structures/vob_map_set.rs

+        self.insert(key, val);
+    }
+
+    /// Insert `val` into the set for key `key`. `v` must not be larger than the set size.


Wrong comment?

Oops. Yeah!

ltratt · 2019-03-25T13:09:38Z

src/librustc_data_structures/vob_map_set.rs

+///   structure cannot reliably express the absence of a key.
+pub struct VobMapSet {
+    vob: Vob,
+    set_size: usize,


Isn't this always the same size as vob.len()?

No. It's the size of the set for each key. vob.len() will be this number multiplied by the number of keys.

ltratt · 2019-03-25T13:12:48Z

src/librustc_data_structures/vob_map_set.rs

+        let mut itr = self.vob.iter().skip(start);
+        for v in 0..self.set_size {
+            if itr.next().unwrap() {
+                self.vob.set(start.checked_add(v).unwrap(), false);


You don't need checked_add here, as it clearly can't wrap (otherwise the Vob wouldn't have been created).

OK, but it looks like this code will disappear due to your previous suggestion.

ltratt · 2019-03-25T13:15:20Z

src/librustc_data_structures/vob_map_set.rs

+    }
+
+    pub fn pop(&mut self, key: usize) -> Option<usize> {
+        let start = Self::idx(self.set_size, key, 0);


Isn't this roughly equivalent to something like:

self.vob.iter_set_bits(self.set_size * ..., self.set_size * ...).next().unwrap_or(None)

?

Ah ha! Yes, that'd be neater.

ltratt · 2019-03-25T13:17:32Z

Having looked at this, I'm not sure this is an improvement overall :/ It will be faster, but we've had to add a lot of code for that, and I'm not sure it's worth the maintenance headache. This is clearly my fault for suggesting this in the first place, so sorry for that!

Maybe the Vec<BitSet> approach was the right compromise between speed and simplicity?

vext01 · 2019-03-25T13:33:40Z

This is clearly my fault for suggesting this in the first place, so sorry for that!

No worries. No harm done. At least we can say we tried :)

Maybe the Vec approach was the right compromise between speed and simplicity?

If this code was in the performance critical path, I think we might have been doing the right thing, but in this case, I can revert the last change without losing sleep.

Sounds like a plan?

ltratt · 2019-03-25T13:34:22Z

Yes, I think reverting is the right thing. Sorry :(

vext01 · 2019-03-25T13:36:56Z

Is it squash time now?

ltratt · 2019-03-25T13:38:55Z

Please squash.

This change: - Starts adding support for statements to TIR. Only simple assignments are translated. Everything else is made into an "unimplemented" statement. - Does the first part of the SSA conversion, namely: * Computes the dominance frontier. * Inserts PHI nodes in the right place and for the correct TIR variables. So what we get is a CFG with PHIs, but which is not yet in SSA. The remaining step is to walk the dominator tree and assign each definition of a variable with a new SSA variable name. Manual inspection of CFG graphs with assignments and PHIs shown look good, but tests will follow later.

vext01 · 2019-03-25T13:43:23Z

Squashed and revised commit message.

ltratt · 2019-03-25T14:28:38Z

bors r+

10: Start converting MIR to SSA TIR. r=ltratt a=vext01 This change: - Starts adding support for statements to TIR. Only simple assignments are translated. Everything else is made into an "unimplemented" statement. The plan is to keep going until the BF interpreter has none of these. - Does the first part of the SSA conversion, namely: * Computes the dominance frontier. * Inserts PHI nodes in the right place and for the correct TIR variables. So what we get is a CFG with PHIs, but which is not yet in SSA. The remaining step is to walk the dominator tree and assign each definition of a variable with a new SSA variable name. That's yet another algorithm from the Appel book. To keep PRs manageable, I think we should do that as a separate PR. Manual inspection of CFG graphs with assignments and PHIs shown look good. Here's a couple of examples: ## example 1 ![image](https://user-images.githubusercontent.com/604955/54438832-6ed50500-472f-11e9-9dcc-c63a01c2dea4.png) ## example 2 ![image](https://user-images.githubusercontent.com/604955/54438860-7e544e00-472f-11e9-8bab-e44335fa63c1.png) I'd like to add some tests to this code to try and weed out some early bugs (I'm sure there will be some). This is likely to take me a little while as I want to inspect how upstream does this kind of testing. I want to avoid manually constructing MIRs and TIRs by hand if I can help it. Question: Currently PHI nodes are of arbitrary arity to make them easier to read during development. We talked about having only binary PHIs. Is that still what we want? [There is a local ykpack Cargo path until we decide what to do with PHIs arity] Cheers. Co-authored-by: Edd Barrett <[email protected]>

bors · 2019-03-25T20:49:05Z

Build succeeded

buildbot/buildbot-build-script

run test for vxWorks in 'pure' static linking mode by default;

This is a combination of 18 commits. Commit softdevteam#2: Additional examples and some small improvements. Commit softdevteam#3: fixed mir-opt non-mir extensions and spanview title elements Corrected a fairly recent assumption in runtest.rs that all MIR dump files end in .mir. (It was appending .mir to the graphviz .dot and spanview .html file names when generating blessed output files. That also left outdated files in the baseline alongside the files with the incorrect names, which I've now removed.) Updated spanview HTML title elements to match their content, replacing a hardcoded and incorrect name that was left in accidentally when originally submitted. Commit softdevteam#4: added more test examples also improved Makefiles with support for non-zero exit status and to force validation of tests unless a specific test overrides it with a specific comment. Commit softdevteam#5: Fixed rare issues after testing on real-world crate Commit softdevteam#6: Addressed PR feedback, and removed temporary -Zexperimental-coverage -Zinstrument-coverage once again supports the latest capabilities of LLVM instrprof coverage instrumentation. Also fixed a bug in spanview. Commit softdevteam#7: Fix closure handling, add tests for closures and inner items And cleaned up other tests for consistency, and to make it more clear where spans start/end by breaking up lines. Commit softdevteam#8: renamed "typical" test results "expected" Now that the `llvm-cov show` tests are improved to normally expect matching actuals, and to allow individual tests to override that expectation. Commit softdevteam#9: test coverage of inline generic struct function Commit softdevteam#10: Addressed review feedback * Removed unnecessary Unreachable filter. * Replaced a match wildcard with remining variants. * Added more comments to help clarify the role of successors() in the CFG traversal Commit softdevteam#11: refactoring based on feedback * refactored `fn coverage_spans()`. * changed the way I expand an empty coverage span to improve performance * fixed a typo that I had accidently left in, in visit.rs Commit softdevteam#12: Optimized use of SourceMap and SourceFile Commit softdevteam#13: Fixed a regression, and synched with upstream Some generated test file names changed due to some new change upstream. Commit softdevteam#14: Stripping out crate disambiguators from demangled names These can vary depending on the test platform. Commit softdevteam#15: Ignore llvm-cov show diff on test with generics, expand IO error message Tests with generics produce llvm-cov show results with demangled names that can include an unstable "crate disambiguator" (hex value). The value changes when run in the Rust CI Windows environment. I added a sed filter to strip them out (in a prior commit), but sed also appears to fail in the same environment. Until I can figure out a workaround, I'm just going to ignore this specific test result. I added a FIXME to follow up later, but it's not that critical. I also saw an error with Windows GNU, but the IO error did not specify a path for the directory or file that triggered the error. I updated the error messages to provide more info for next, time but also noticed some other tests with similar steps did not fail. Looks spurious. Commit softdevteam#16: Modify rust-demangler to strip disambiguators by default Commit softdevteam#17: Remove std::process::exit from coverage tests Due to Issue #77553, programs that call std::process::exit() do not generate coverage results on Windows MSVC. Commit softdevteam#18: fix: test file paths exceeding Windows max path len

vext01 commented Mar 15, 2019

View reviewed changes

vext01 assigned vext01 and ltratt and unassigned vext01 Mar 15, 2019

ltratt reviewed Mar 15, 2019

View reviewed changes

ltratt reviewed Mar 18, 2019

View reviewed changes

vext01 commented Mar 25, 2019

View reviewed changes

ltratt reviewed Mar 25, 2019

View reviewed changes

vext01 force-pushed the yk-tir-stmts branch from 9009fcf to aba3df7 Compare March 25, 2019 13:43

bors bot merged commit aba3df7 into master Mar 25, 2019

bors bot deleted the yk-tir-stmts branch March 25, 2019 20:49

bors bot pushed a commit that referenced this pull request Sep 22, 2019

Merge pull request #10 from Wind-River/bpang-runtest

964c37c

run test for vxWorks in 'pure' static linking mode by default;


		// FIXME - rename variables with fresh SSA names.

		// Put the finalised TIR to disk.

Start converting MIR to SSA TIR. #10

Start converting MIR to SSA TIR. #10

Uh oh!

Conversation

vext01 commented Mar 15, 2019

example 1

example 2

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 commented Mar 15, 2019

Uh oh!

ltratt commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ltratt commented Mar 15, 2019 •

edited

Loading