Skip to content

Disable LLVM Global Isel#9401

Merged
bcardiff merged 1 commit intocrystal-lang:masterfrom
jhass:disable_global_isel
Jun 29, 2020
Merged

Disable LLVM Global Isel#9401
bcardiff merged 1 commit intocrystal-lang:masterfrom
jhass:disable_global_isel

Conversation

@jhass
Copy link
Member

@jhass jhass commented Jun 1, 2020

It's not enabled by default on x86, but on other targets it is. At least until https://reviews.llvm.org/D80898 is released, it doesn't like us generating a value of a zero sized type

We may need to fix doing that, but until then this workarounds the issue.

See #9297 (comment) for more background.

It's not enabled by default on x86, but on other targets it is.
At least until https://reviews.llvm.org/D80898 is released, it
doesn't like us generating a value of a zero sized type

We may need to fix doing that, but until then this workarounds
the issue.

See crystal-lang#9297 (comment)
for more background.
@jhass jhass added platform:aarch64 kind:bug A bug in the code. Does not apply to documentation, specs, etc. labels Jun 4, 2020
@jhass jhass mentioned this pull request Jun 8, 2020
@jhass
Copy link
Member Author

jhass commented Jun 16, 2020

Oh yeah, fixes #8080 / worksaround (I didn't know we had an issue ticket for this).

@waj
Copy link
Member

waj commented Jun 22, 2020

@jhass is it GlobalIsel used in x86_64? Do you know what are the zero sized values that are currently being generated?

@jhass
Copy link
Member Author

jhass commented Jun 22, 2020

According to the hints I got in IRC it's not by default and barely implemented there.

I don't know for certain why they are generated, but I suspect something around nil.

I found it through bugpoint, so it's a bit hard to correlate back to the original code.

@asterite
Copy link
Member

nil is such a type:

@nil_type ||= @structs["Nil"] ||= @llvm_context.struct([] of LLVM::Type, "Nil")

@jhass
Copy link
Member Author

jhass commented Jun 22, 2020

Yeah, I forgot, in the comment linked above I actually related it back to (an instance of) the original code. So it has to do with nil return type procs.

@asterite
Copy link
Member

Strange, for nil return type procs I would expect that to translate to void, but maybe I'm wrong.

Also, nothing prevents a user from creating an empty struct...

@jhass
Copy link
Member Author

jhass commented Jun 22, 2020

Also LLVM acknowledged and fixed this as a bug, so while I got a few why the hell do you do this in response, it seems legit if we do the easy thing right now and apply this workaround.

@waj
Copy link
Member

waj commented Jun 22, 2020

Ok, but what's the current implication of disabling it? Are we going to get less performance in some platforms or what?

@jhass
Copy link
Member Author

jhass commented Jun 22, 2020

Maybe? But I think it's only slightly and a big point is also an architectural change within LLVM allowing them to do more or different things in the future.

https://releases.llvm.org/9.0.0/docs/GlobalISel.html

@jhass jhass added the pr:ready-to-merge The changes are good to go, we need to triage merging it. label Jun 22, 2020
@jhass jhass added this to the 1.0.0 milestone Jun 29, 2020
@bcardiff bcardiff removed the pr:ready-to-merge The changes are good to go, we need to triage merging it. label Jun 29, 2020
@bcardiff bcardiff merged commit 56ab086 into crystal-lang:master Jun 29, 2020
@jhass jhass deleted the disable_global_isel branch June 29, 2020 15:04
@bcardiff bcardiff mentioned this pull request Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:aarch64

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants