Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Slot a Union type #27427

Closed
wants to merge 1 commit into from
Closed

Make Slot a Union type #27427

wants to merge 1 commit into from

Conversation

Liozou
Copy link
Member

@Liozou Liozou commented Jun 4, 2018

Closes #27202

I'm not familiar with these files, so it would be good if someone checked I haven't broken anything by moving Slot from the C builtins to boot.jl

@vtjnash
Copy link
Member

vtjnash commented Jun 4, 2018

Nice work! I haven't reviewed in much detail, but I'm amazed how small this is. Very recently though, TypedSlot became unused (outside of a brief appearance in the IR between inlining and optimization in Core.Compiler). We can still merge this, but if you're able to completely remove all references to it outside of base/compiler/optimize.jl and base/compiler/ssair/slot2ssa.jl, that would be even more epic! :)

@Liozou
Copy link
Member Author

Liozou commented Jun 4, 2018

On master TypedSlot seems to be still used in boot.jl and compiler/utilities.jl so I'm afraid to break everything if I replace Slot by SlotNumber almost everywhere... Is this removal of TypedSlot on a specific branch?

@Liozou
Copy link
Member Author

Liozou commented Jun 4, 2018

Actually, forget about boot.jl, that's not threatening.
I forgot to deal with lines 445 and 449 of boot.jl, but to say the truth I'm not sure to understand what they do. Anyway, should Slot be removed from them?

@vtjnash
Copy link
Member

vtjnash commented Jun 5, 2018

It's essentially executable documentation at

module IR

The work hasn't yet been done to detangle and remove the references to TypedSlot (it's only just recently become feasible).

@Liozou
Copy link
Member Author

Liozou commented Jun 5, 2018

OK, in that case I'm going to have a look at what can be trimmed

@Liozou
Copy link
Member Author

Liozou commented Jun 5, 2018

I made a scan of all the uses of Slot in the code and, apart from optimize.jl, it also mainly appears in typelattice.jl and abstractinterpretation.jl. I suppose you would like to replace it with SlotNumber as much as possible there, but I have no idea whether that would be correct in these cases since I don't know this part of the compiler at all.
As for TypedSlot itself, I remarked that it was used to define slot_id twice: once in utilities.jl and the other in ssair/slot2ssa.jl. Should one of the two be removed?

@vtjnash
Copy link
Member

vtjnash commented Jun 5, 2018

It's referenced (but will never actually appear) in abstractinterpretation.jl. It might appear in the typelattice, but you can define TypedSlot in that code (in Core.Compiler, and not a subtype of Slot) and handle it in the few cases where it is observed to appear.

@Liozou
Copy link
Member Author

Liozou commented Jun 5, 2018

TypedSlot never explicitly appears in abstractinterpretation.jl, only Slot does and I don't know whether it can or cannot contain a TypedSlot value. This is kind of my issue whenever Slot is used: I never know whether it is safe to replace it by SlotNumber or not.

@aviatesk
Copy link
Member

Will be replaced by #48693.

@aviatesk aviatesk closed this Feb 16, 2023
@Liozou Liozou deleted the slotunion branch February 17, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Core.Slot a Union type instead of a generic abstract type
3 participants