Stream: t-compiler

Topic: ad hoc triage meeting 2020.04.10 #54818


pnkfelix (Apr 10 2020 at 14:26, on Zulip):

first, nominations: https://github.com/rust-lang/rust/issues?q=is%3Aopen+label%3AI-nominated+label%3AT-compiler

pnkfelix (Apr 10 2020 at 14:26, on Zulip):

nom 1/8: "ICEs should always print the top of the query stack." #70953

pnkfelix (Apr 10 2020 at 14:26, on Zulip):

we discussed #70953 yesterday I believe

eddyb (Apr 10 2020 at 14:26, on Zulip):

I think we w- yeah

pnkfelix (Apr 10 2020 at 14:27, on Zulip):

(just trying to be thorough/mechanical, as I am a replicant after all)

eddyb (Apr 10 2020 at 14:27, on Zulip):

@bjorn3 suggested always emitting backtraces but I recall there being issues with that. but maybe they've been fixed

eddyb (Apr 10 2020 at 14:27, on Zulip):

(e.g. libbacktrace used to be much slower IIRC)

pnkfelix (Apr 10 2020 at 14:27, on Zulip):

yeah. I think there was general support for the idea? Anyway if anyone objects, please post on issue itself

pnkfelix (Apr 10 2020 at 14:27, on Zulip):

nom 2/8: "Fix staticlib name for *-pc-windows-gnu targets" #70937

centril (Apr 10 2020 at 14:27, on Zulip):

If there are no major issues, that would be great to always print it :+1:

pnkfelix (Apr 10 2020 at 14:27, on Zulip):

we also discussed #70937 yesterday

pnkfelix (Apr 10 2020 at 14:28, on Zulip):

there were no objections but a general feeling that this area needs an owner?

pnkfelix (Apr 10 2020 at 14:28, on Zulip):

so if you have objections to #70937, please post on the ticket

centril (Apr 10 2020 at 14:28, on Zulip):

Maybe we should reach out to the winapi crate owner

pnkfelix (Apr 10 2020 at 14:29, on Zulip):

is that retep?

centril (Apr 10 2020 at 14:29, on Zulip):

we could use more windows experts

centril (Apr 10 2020 at 14:29, on Zulip):

@pnkfelix believe so

pnkfelix (Apr 10 2020 at 14:29, on Zulip):

about this specific change, or about being a windows owner?

centril (Apr 10 2020 at 14:29, on Zulip):

windows owner

Wesley Wiser (Apr 10 2020 at 14:29, on Zulip):

They got cc'd on the issue

centril (Apr 10 2020 at 14:29, on Zulip):

ah :+1:

eddyb (Apr 10 2020 at 14:29, on Zulip):

I think in the past we've let e.g. acrichto handle stuff like this but it always lead to having a lot on their plate

centril (Apr 10 2020 at 14:30, on Zulip):

They have less on their plate now :slight_smile:

pnkfelix (Apr 10 2020 at 14:30, on Zulip):

I can reach out to retep, see if they object to being our domain expert here

pnkfelix (Apr 10 2020 at 14:30, on Zulip):

(I think achricto has plenty on their plate, its just hidden behind NDAs)

pnkfelix (Apr 10 2020 at 14:30, on Zulip):

but what do I know

eddyb (Apr 10 2020 at 14:31, on Zulip):

IOW I think having dedicated experts is better than just throwing everything nobody else in the compiler team can handle in the general direction of one person

pnkfelix (Apr 10 2020 at 14:31, on Zulip):

nom 3/8: "[WIP] Rename mir::Rvalue to Op." #70928

eddyb (Apr 10 2020 at 14:31, on Zulip):

oh boy this one

pnkfelix (Apr 10 2020 at 14:31, on Zulip):

what was motivation here?

eddyb (Apr 10 2020 at 14:31, on Zulip):

so we started doing some renames years ago

centril (Apr 10 2020 at 14:31, on Zulip):

My reaction to this PR is sorta: "but why?"

eddyb (Apr 10 2020 at 14:31, on Zulip):

we got rid of "lvalue" and "rvalue" in rustc

eddyb (Apr 10 2020 at 14:32, on Zulip):

except we sort of left mir::Rvalue behind

centril (Apr 10 2020 at 14:32, on Zulip):

We use Value vs. Place in the reference and UCG

eddyb (Apr 10 2020 at 14:32, on Zulip):

so it's just sitting there, orphaned from "official terminology"

eddyb (Apr 10 2020 at 14:32, on Zulip):

right

centril (Apr 10 2020 at 14:32, on Zulip):

(I didn't like it much, but I've accepted it ^^)

pnkfelix (Apr 10 2020 at 14:32, on Zulip):

I think @centril 's argument on the ticket that Op is far harder to grep for than Rvalue is a very strong case

eddyb (Apr 10 2020 at 14:33, on Zulip):

\bOp\b :P?

eddyb (Apr 10 2020 at 14:33, on Zulip):

but okay yeah it's probably too short

centril (Apr 10 2020 at 14:33, on Zulip):

(in particular because Op is a prefix of Operand)

pnkfelix (Apr 10 2020 at 14:33, on Zulip):

My personal preference is that we leave it be as is

eddyb (Apr 10 2020 at 14:33, on Zulip):

bikeshed-wise I don't care too much what we rename it to, it just reminds me every time I see it of a job not finished

pnkfelix (Apr 10 2020 at 14:33, on Zulip):

(or at least wait until we come up with a better new name)

eddyb (Apr 10 2020 at 14:33, on Zulip):

@pnkfelix hmm can we use an FCP to "vote" on this?

pnkfelix (Apr 10 2020 at 14:33, on Zulip):

I guess so

eddyb (Apr 10 2020 at 14:34, on Zulip):

I guess that doesn't account for the bikeshed aspect

eddyb (Apr 10 2020 at 14:34, on Zulip):

Operation is the direct longform alternative

pnkfelix (Apr 10 2020 at 14:34, on Zulip):

should I do an rfcbot fcp close, or fcp postpone ...

centril (Apr 10 2020 at 14:35, on Zulip):

Does "Operation" make semantic sense?

pnkfelix (Apr 10 2020 at 14:35, on Zulip):

I think close is the right thing here

eddyb (Apr 10 2020 at 14:35, on Zulip):

heh I forget it has that many options

eddyb (Apr 10 2020 at 14:35, on Zulip):

@centril it's an operation on operands that produces a value

centril (Apr 10 2020 at 14:35, on Zulip):

Operation feels more "impure" to me (terminatorkind)

centril (Apr 10 2020 at 14:35, on Zulip):

@eddyb fair

eddyb (Apr 10 2020 at 14:36, on Zulip):

Operator would be confusing because of the expectation of"operator is either unary or binary (but not other things)", but kind of funny to consider

centril (Apr 10 2020 at 14:36, on Zulip):

"Rvalue" seems memorable and sorta unique at least

centril (Apr 10 2020 at 14:36, on Zulip):

Should we move on?

pnkfelix (Apr 10 2020 at 14:37, on Zulip):

(if we're bikeshedding, I'd be curious to know if Expr is taken)

pnkfelix (Apr 10 2020 at 14:37, on Zulip):

anyway

eddyb (Apr 10 2020 at 14:37, on Zulip):

it's not

eddyb (Apr 10 2020 at 14:37, on Zulip):

better to move on, before it sinks in that I should've just finished the job years ago :P

pnkfelix (Apr 10 2020 at 14:37, on Zulip):

nom 4/8: Compile regression "cannot infer an appropriate lifetime for lifetime parameter" #70917

pnkfelix (Apr 10 2020 at 14:37, on Zulip):

this is once again a lang team nom, right? :wink:

eddyb (Apr 10 2020 at 14:38, on Zulip):

pretty sure we discussed it last night

centril (Apr 10 2020 at 14:38, on Zulip):

@pnkfelix lang did discuss; so now it is t-compiler

pnkfelix (Apr 10 2020 at 14:38, on Zulip):

yeah and we're waiting on a crater run

pnkfelix (Apr 10 2020 at 14:38, on Zulip):

so nothing to discuss yet

eddyb (Apr 10 2020 at 14:38, on Zulip):

oh wow I love that ability

pnkfelix (Apr 10 2020 at 14:38, on Zulip):

other than to report the lang team findings

centril (Apr 10 2020 at 14:38, on Zulip):

I will try to make a point-release for proptest

eddyb (Apr 10 2020 at 14:38, on Zulip):

(to just run crater on arbitrary PRs after the fact)

centril (Apr 10 2020 at 14:38, on Zulip):

That's not really "t-compiler", but I guess it is, a bit? :D

pnkfelix (Apr 10 2020 at 14:39, on Zulip):

nom 5/8: forbid overwritten by later allow on the same "scope level" #70819

centril (Apr 10 2020 at 14:39, on Zulip):

Left a question for you on that one

pnkfelix (Apr 10 2020 at 14:39, on Zulip):

wait, I thought I would have remembered if we had talked about this in lang team meeting last night

centril (Apr 10 2020 at 14:39, on Zulip):

we didn't have the time

pnkfelix (Apr 10 2020 at 14:39, on Zulip):

the lang team aspect was the fact that the attribute semantics is visible in the source text

pnkfelix (Apr 10 2020 at 14:40, on Zulip):

so 1. is it a bug about how #![forbid(X)] #![allow(X)] is handled today

centril (Apr 10 2020 at 14:40, on Zulip):

@pnkfelix sure; but is there a design question re. the semantics?

pnkfelix (Apr 10 2020 at 14:40, on Zulip):

and 2. should we strive to ensure the command line lint handling matches the source level attribute lint handling

pnkfelix (Apr 10 2020 at 14:40, on Zulip):

ah I see, @RalfJ has commented further since then

pnkfelix (Apr 10 2020 at 14:41, on Zulip):

and that scope levels are relevant

centril (Apr 10 2020 at 14:41, on Zulip):

I believe both instances Ralf has mentioned re. attributes are bugs

centril (Apr 10 2020 at 14:41, on Zulip):

(in the current semantics)

pnkfelix (Apr 10 2020 at 14:41, on Zulip):

okay well if everyone is probably in agreement that this is indeed a bug

pnkfelix (Apr 10 2020 at 14:42, on Zulip):

then it probably can be retagged as just T-compiler

pnkfelix (Apr 10 2020 at 14:42, on Zulip):

and I'm inclined to remove T-lang from the issue then. I mostly wanted to make sure that they were onboard for changing things here, since I think the attribute (mis)handling is ... old ...?

pnkfelix (Apr 10 2020 at 14:42, on Zulip):

(and that this therefore would be a breaking change, though one that I suspect will not matter much in practice...)

centril (Apr 10 2020 at 14:42, on Zulip):

@pnkfelix happy to discuss it at T-Lang if you wish though

simulacrum (Apr 10 2020 at 14:43, on Zulip):

yeah the only reason why it might break people is if they're doing e.g.

// #![forbid(warnings)]
// #![deny(unused)]
centril (Apr 10 2020 at 14:43, on Zulip):

if you believe that is necessary; but it would be good to leave a clarifying comment on issue though

pnkfelix (Apr 10 2020 at 14:43, on Zulip):

shrug

pnkfelix (Apr 10 2020 at 14:43, on Zulip):

I do think a clarifying comment is necessary

pnkfelix (Apr 10 2020 at 14:43, on Zulip):

and I no longer am certain it needs discussion at T-lang meeting

centril (Apr 10 2020 at 14:43, on Zulip):

(N.B. this is a lint, so not breaking in the RFC 1122 sense)

pnkfelix (Apr 10 2020 at 14:43, on Zulip):

I'll follow up later.

pnkfelix (Apr 10 2020 at 14:44, on Zulip):

Its in the handling of lints... it seems like thats more fundamental, no?

pnkfelix (Apr 10 2020 at 14:44, on Zulip):

maybe I misunderstnad. but we don't need to debate that now

centril (Apr 10 2020 at 14:44, on Zulip):

alright

pnkfelix (Apr 10 2020 at 14:44, on Zulip):

nom 6/8: "WIP toward LLVM Code Coverage for Rust" #70680

centril (Apr 10 2020 at 14:45, on Zulip):

I think this one should be bumped to MCP / design meeting

centril (Apr 10 2020 at 14:45, on Zulip):

@eddyb has some concerns

pnkfelix (Apr 10 2020 at 14:45, on Zulip):

yeah I don't think we can resolve the Q's here in a quick triage session

pnkfelix (Apr 10 2020 at 14:45, on Zulip):

I suspect a MCP will fit

pnkfelix (Apr 10 2020 at 14:45, on Zulip):

but ... it also seems like it is undergoing active redesign?

eddyb (Apr 10 2020 at 14:46, on Zulip):

MCP would make me happy

centril (Apr 10 2020 at 14:46, on Zulip):

Lang team and libs team might need to be consulted, if this is ever to reach stable

pnkfelix (Apr 10 2020 at 14:46, on Zulip):

(based on feedback provided)

eddyb (Apr 10 2020 at 14:46, on Zulip):

I have opinions because I almost got paid to do this in the past and did research on approaches and technical details

pnkfelix (Apr 10 2020 at 14:46, on Zulip):

so maybe the right answer here is to 1. advertise this PR to the T-compiler team at large (which we are doing here), and 2. tell PR author that we think a MCP is warranted.

eddyb (Apr 10 2020 at 14:46, on Zulip):

but it's probably more important that other (than me) people think the design is good

centril (Apr 10 2020 at 14:46, on Zulip):

@pnkfelix sgtm

pnkfelix (Apr 10 2020 at 14:46, on Zulip):

okay. I can follow up there

pnkfelix (Apr 10 2020 at 14:47, on Zulip):

nom 7/8: "Use the niche optimization if other variant are small enough" #70477

centril (Apr 10 2020 at 14:47, on Zulip):

@eddyb I think your concerns are pretty valid and important that we listen to you in particular

pnkfelix (Apr 10 2020 at 14:47, on Zulip):

oh #70477 is pretty cool

eddyb (Apr 10 2020 at 14:47, on Zulip):

but I don't have the energy to fight people who want to their own thing

eddyb (Apr 10 2020 at 14:48, on Zulip):

at least with #70477 I can keep finding edge cases for them to consider :P

eddyb (Apr 10 2020 at 14:48, on Zulip):

@pnkfelix see the two bullet-points in https://github.com/rust-lang/rust/pull/70477#issuecomment-607725781

eddyb (Apr 10 2020 at 14:49, on Zulip):

that is my main concern with their approach

pnkfelix (Apr 10 2020 at 14:49, on Zulip):

yeah I saw that comment and considered quoting it here, thanks for linking it

pnkfelix (Apr 10 2020 at 14:49, on Zulip):

so it seems like part of the issue (maybe the only issue) ...

pnkfelix (Apr 10 2020 at 14:49, on Zulip):

is that the approached encoded in the PR attempts to ensure that it always picks a better choice

centril (Apr 10 2020 at 14:50, on Zulip):

I want to land this, but I'm not comfortable with it unless one of these two things happen:
- the implementation gets a formal model, where we can prove things about all possible cases
- we check instead of assuming the conditions are sufficient

I think this is pretty reasonable; more soundness holes is not something we need atm :slight_smile:

pnkfelix (Apr 10 2020 at 14:50, on Zulip):

but it is unproven that it is actually improving

eddyb (Apr 10 2020 at 14:50, on Zulip):

@centril at least no layout should be unsound :P

pnkfelix (Apr 10 2020 at 14:50, on Zulip):

but to be clear

pnkfelix (Apr 10 2020 at 14:50, on Zulip):

its not injecting soundness holes

pnkfelix (Apr 10 2020 at 14:50, on Zulip):

just potential size regressions, right?

pnkfelix (Apr 10 2020 at 14:50, on Zulip):

(which is bad, but not bad on the level of unsound)

pnkfelix (Apr 10 2020 at 14:50, on Zulip):

and the approach @eddyb has been advocating

centril (Apr 10 2020 at 14:51, on Zulip):

oh; if that's the case then...

pnkfelix (Apr 10 2020 at 14:51, on Zulip):

is instead to just compute "both" layouts (which in general ... might be N layouts, no?)

pnkfelix (Apr 10 2020 at 14:51, on Zulip):

and choose the best one? Does that sound right?

eddyb (Apr 10 2020 at 14:51, on Zulip):

yeah

centril (Apr 10 2020 at 14:51, on Zulip):

So we would speculate, that is

eddyb (Apr 10 2020 at 14:51, on Zulip):

the reason I even bring up "checking instead of assuming" is that I've tried to come up with all the conditions for doing this the "clever predicate" way

eddyb (Apr 10 2020 at 14:52, on Zulip):

and it's a nightmare to reason about

eddyb (Apr 10 2020 at 14:52, on Zulip):

and in other parts of the compiler we just collect "candidates" and pick from there

pnkfelix (Apr 10 2020 at 14:52, on Zulip):

I think @nikomatsakis chimed in on the ticket with support for @eddyb 's feedback

eddyb (Apr 10 2020 at 14:52, on Zulip):

which I respect even more now, that I know places where not doing that has caused a lot of wasted time trying to think through "clever decision-making logic"

pnkfelix (Apr 10 2020 at 14:52, on Zulip):

and I am of a similar mindset. Even if the PR author believes the conditions they have encoded are easy enough to reason about, we need to think of long term maintenance

eddyb (Apr 10 2020 at 14:53, on Zulip):

another thing is that I've received complaints over the years that the layout code is a complex mess

pnkfelix (Apr 10 2020 at 14:53, on Zulip):

the only concern I have is

pnkfelix (Apr 10 2020 at 14:53, on Zulip):

how expensive is it to try multiple layouts?

eddyb (Apr 10 2020 at 14:53, on Zulip):

which is mostly a byproduct of what it grew out of (the old straight-to-LLVM ADT layout logic)

eddyb (Apr 10 2020 at 14:53, on Zulip):

@pnkfelix I think it would be easy to measure that, just nobody has done it

pnkfelix (Apr 10 2020 at 14:53, on Zulip):

it should not have downstream effects, at least

pnkfelix (Apr 10 2020 at 14:54, on Zulip):

i.e. I was briefly worried about exponential blowup

pnkfelix (Apr 10 2020 at 14:54, on Zulip):

from nested enum types

eddyb (Apr 10 2020 at 14:54, on Zulip):

yeah, no, once you pick a layout it's done

pnkfelix (Apr 10 2020 at 14:54, on Zulip):

but that should not occur, because we should compute the layout for an enum and stick with it regardless of any context it occurs in

pnkfelix (Apr 10 2020 at 14:54, on Zulip):

so yeah

pnkfelix (Apr 10 2020 at 14:55, on Zulip):

I'm happy to leave a note here too

eddyb (Apr 10 2020 at 14:55, on Zulip):

the way we'd measure this is to not early-return if we found a niche candidate, but just stash it in a variable and check it after we compute the regular layout

pnkfelix (Apr 10 2020 at 14:55, on Zulip):

its a little worrisome that the PR author has been so adamantly opposed to incorporating your suggestion

eddyb (Apr 10 2020 at 14:55, on Zulip):

I may have even suggested this exact thing to the PR author or someone else in the past and nobody has tried it

eddyb (Apr 10 2020 at 14:55, on Zulip):

/me should just go for it, it's like, what, a 5-line change?

eddyb (Apr 10 2020 at 14:55, on Zulip):

(this is how I can spend weeks in weeds)

pnkfelix (Apr 10 2020 at 14:56, on Zulip):

anyway, I can leave a note here too

eddyb (Apr 10 2020 at 14:56, on Zulip):

pnkfelix said:

its a little worrisome that the PR author has been so adamantly opposed to incorporating your suggestion

at least I'm glad that they try to understand and resolve every edge case I throw at them

pnkfelix (Apr 10 2020 at 14:56, on Zulip):

after I read the comment thread more carefully, to better understand the PR author's Point of View

pnkfelix (Apr 10 2020 at 14:56, on Zulip):

nom 8/8: "Should enum discriminants have generics in scope?" #70453

pnkfelix (Apr 10 2020 at 14:57, on Zulip):

this is once again a lang team nomination

pnkfelix (Apr 10 2020 at 14:57, on Zulip):

I believe

centril (Apr 10 2020 at 14:57, on Zulip):

yeah

centril (Apr 10 2020 at 14:57, on Zulip):

this time just to check your boxes

pnkfelix (Apr 10 2020 at 14:57, on Zulip):

(whcih was discussed last night and is in lang team prompt for FCP now)

eddyb (Apr 10 2020 at 14:57, on Zulip):

btw if someone is bored, here's a bit of layout math that the author led me to find out https://github.com/rust-lang/rust/pull/70477#discussion_r403269107

centril (Apr 10 2020 at 14:57, on Zulip):

so e.g., @pnkfelix's box, hint hint :slight_smile:

pnkfelix (Apr 10 2020 at 14:57, on Zulip):

so okay, that's all the nominations and we're basically out of time

pnkfelix (Apr 10 2020 at 14:58, on Zulip):

I'll go back through the notes here and write up the comments I promised.

eddyb (Apr 10 2020 at 14:58, on Zulip):

someone should write a paper on the algebra of layouts

centril (Apr 10 2020 at 14:58, on Zulip):

Excellent, thanks @pnkfelix

eddyb (Apr 10 2020 at 14:58, on Zulip):

and how alignments interact with everything

centril (Apr 10 2020 at 14:58, on Zulip):

@eddyb interesting; seems like that could be a MSc thesis even

centril (Apr 10 2020 at 14:59, on Zulip):

or BSc if not

eddyb (Apr 10 2020 at 14:59, on Zulip):

(it's likely that on average, most permutations of field order result in the same number of padding bytes, it's really hard to even make examples which go against that)

centril (Apr 10 2020 at 15:00, on Zulip):

:wave: y'all

eddyb (Apr 12 2020 at 11:41, on Zulip):

pnkfelix said:

and I am of a similar mindset. Even if the PR author believes the conditions they have encoded are easy enough to reason about, we need to think of long term maintenance

@pnkfelix @nikomatsakis so here's some good news, I went and tried doing the most minimal "two layouts cost measurement" version of this, and it's largely perf-neutral: https://github.com/rust-lang/rust/pull/71045#issuecomment-612579390

eddyb (Apr 12 2020 at 11:42, on Zulip):

the PR author also did something similar, but got a regression: https://github.com/rust-lang/rust/pull/70477#issuecomment-612493899

eddyb (Apr 12 2020 at 11:42, on Zulip):

so we now have to figure out where the slowdown comes from. which seems productive

Peter Rabbit (Apr 20 2020 at 01:28, on Zulip):

pnkfelix said:

I can reach out to retep, see if they object to being our domain expert here

I don't object.

Last update: Jun 04 2020 at 17:30UTC