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

Filter out constraint tuple class constructors in hasNoTypeClasses #75

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

Mikolaj
Copy link
Contributor

@Mikolaj Mikolaj commented Aug 19, 2023

A rough sketch. I don't know what I'm doing. See #74.

@nomeata
Copy link
Owner

nomeata commented Aug 20, 2023

Thanks, decent start, I can clean up it a bit before merging. But I’ll wait for the discussion in #74, to see if all your issues can be resolved.

@Mikolaj
Copy link
Contributor Author

Mikolaj commented Aug 20, 2023

I'm additionally filtering out heqTyConName and eqTyConName, because both appear in my code, I think. Now inspection-testing no longer complains about ~ classes spotted, so it seems to work. Let me know if that's a bad idea.

@Mikolaj
Copy link
Contributor Author

Mikolaj commented Aug 20, 2023

I've reverted the last change --- it works great with ''(~) --- I had no idea to try something like that.

@nomeata
Copy link
Owner

nomeata commented Aug 20, 2023

Can you do it for the tuples as well? (Mostly curious, it might still be good to do so by default)

@Mikolaj
Copy link
Contributor Author

Mikolaj commented Aug 20, 2023

Any idea how to try? I did try adding ''(%,,,,,,,,%) to the list and ''$d(%,,,,,,,,%) and ''d(%,,,,,,,,%) and each is rejected with "parse error on input ‘,’".

@nomeata
Copy link
Owner

nomeata commented Aug 20, 2023

I would have expected the first to work. Nevermind, not that important.

@Mikolaj
Copy link
Contributor Author

Mikolaj commented Aug 22, 2023

I don't think I will have any more actionable input for now.

Your tool, after patching, helped me manually specialize my code much more, to the point I've discovered a new bug in GHC and/or plugins (https://gitlab.haskell.org/ghc/ghc/-/issues/23866), which makes GHC 9.4 unusable for me. I still have one GHC version that works correctly, GHC 9.2, though it probably doesn't specialize as well --- I can't tell, because I needed to add as exceptions all classes that constrain existential variables, so I can't easily detect failures to eliminate these dictionaries in non-existential contests (the lack of an explicit exists quantifier in Core and in Haskell surface language doesn't help, either):

inspect $ hasNoTypeClassesExcept 'revRankedListProd [''GoodScalar, ''KnownNat, ''OS.Shape, ''AstSpan, ''Show, ''Ord, ''Numeric, ''Num, ''RowSum, ''Typeable, ''IfDifferentiable, ''NFData, ''RealFrac, ''Integral, ''Fractional, ''RealFloat, ''Floating]  

BTW, I've also just discovered, using your tool, than GHC 9.8.1-alpha1 specializes worse than GHC 9.4, even in its unsound -fpolymorphic-specialisation mode (https://gitlab.haskell.org/ghc/ghc/-/issues/23874). [Edit: I was wrong, this is not a regression; GHC 9.4 gave up specialization even earlier, which is why there was no mention of IsPrimal class in the benchmark file, which is why your tool did not report problems --- this is the non-whole-program nature of this analysis showing its limitations. I'd need to add hasNoTypeClassesExcept to ~5 modules to make sure I catch non-specialized IsPrimal regardless of where GHC gets stuck specializing it.]

@nomeata
Copy link
Owner

nomeata commented Aug 22, 2023

Well, that's at least something :-)

@Mikolaj
Copy link
Contributor Author

Mikolaj commented Sep 12, 2023

So, does it have a chance to get merged? :)

@Mikolaj
Copy link
Contributor Author

Mikolaj commented Sep 13, 2023

Oh, yes, the CI fails, because I cheated for the old GHCs. :)

@Mikolaj
Copy link
Contributor Author

Mikolaj commented Sep 13, 2023

Thank you!

@nomeata nomeata merged commit 56d9e34 into nomeata:master Sep 13, 2023
10 checks passed
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.

2 participants