Stream: t-compiler/major changes

Topic: Remove Spans from HIR compiler-team#294


triagebot (May 19 2020 at 11:09, on Zulip):

A new proposal has been announced #294. It will be brought up at the next meeting.

nikomatsakis (May 19 2020 at 17:27, on Zulip):

@cjgillot what is the latest estimate of perf impact on this?

cjgillot (May 19 2020 at 17:32, on Zulip):

I lost access to my other computer, so I can't measure locally. I just pushed a hopefully faster solution.

pnkfelix (May 21 2020 at 20:43, on Zulip):

hey @cjgillot , I wanted to double-check with you about some aspects of this MCP

pnkfelix (May 21 2020 at 20:45, on Zulip):

in particular, one thing I need to straighten out in my own mind is how we get the desired benefit (more incremental compilation) while not losing any current features. Specifically: when code layout (and thus spans) change in ways that otherwise do not affect semantics, what is your expectation? That the HIR will be unchanged (and computational queries based on it reused), but the side-table will be updated?

pnkfelix (May 21 2020 at 20:45, on Zulip):

Is the above is correct, does your preliminary #72015 actually implement such side-table updating?

cjgillot (May 21 2020 at 21:26, on Zulip):

Yes, the idea is to have the HIR remain the same, and only the side table should change.
#72015 implements this partially: I have not removed all the spans from HIR. For now, I only handle those with an associated HirId.

cjgillot (May 21 2020 at 21:30, on Zulip):

Actually, after the 5th commit in #72015, the side table is completely built. All the other commits are dedicated to removing spans from the HIR.

triagebot (May 22 2020 at 01:35, on Zulip):

@T-compiler: Proposal #294 has been seconded, and will be approved in 10 days if no objections are raised.

Amanieu (May 28 2020 at 15:47, on Zulip):

There might be potential interactions with #72625 which adds a Vec<Span> to InlineAsm HIR and MIR nodes that contains the source spans for each line of the inline asm. This is passed to LLVM in codegen which allows assembler errors to be mapped back to a source line.

Amanieu (May 28 2020 at 15:52, on Zulip):

Also InlineAsmTemplatePiece is passed through all of the layers (HIR, MIR) and contains a Span that points to the individual placeholders ({}) in the asm template string. This span is only used for diagnostics in AST lowering and in the intrinsicsck pass (just after typeck, before MIR).

cjgillot (May 28 2020 at 16:12, on Zulip):

I don't think this is a pressing issue. Inline ASM has very specific needs, and is not of very wide use, so it is at the end of my roadmap. I will look for a solution, but keeping the Vec<Span> should be fine.

cjgillot (Jun 11 2020 at 07:09, on Zulip):

I am stuck with the regression in #72878. Stable hashing of the spans seems to be one of the reasons. Does somebody have a suggestion on how to reduce its impact ?

nikomatsakis (Jun 15 2020 at 21:20, on Zulip):

@cjgillot I'm not sure, but can you say a bit more about why we have to stable hash the spans? Is it that we are hashing the spans that we see as the return type of queries? I guess I have to review the overall plan here, I forget the details.

nikomatsakis (Jun 15 2020 at 21:21, on Zulip):

When talking to @Nell Shamrell-Harrington today about an unrelated issue, I realized that an advantage of this approach is that, whenever you have a span, you would also have the hir_id of an expression, which is potentially quite useful in error reporting, since you can then analyze the source expression in a more structured way (cc @Esteban K├╝ber).

nikomatsakis (Jun 15 2020 at 21:22, on Zulip):

Well, maybe not whenever, but more often anyway.

cjgillot (Jun 15 2020 at 21:22, on Zulip):

Yes, it happens at the return of the query.

cjgillot (Jun 15 2020 at 21:24, on Zulip):

The query is indexed using a local defid, and returns a vector of all the spans it owns. The latter can be sizeable.

cjgillot (Jun 15 2020 at 21:24, on Zulip):

There may be other sources of slowness. This the one I have identified.

cjgillot (Jun 23 2020 at 10:40, on Zulip):

Hi, @nikomatsakis @pnkfelix. I did not manage to remove the perf regression. I still have a ~4% regression (~ 10ms) on many benchmarks. I would welcome some ideas.

Last update: May 07 2021 at 07:45UTC