snowleopard / hadrian

Hadrian: a new build system for the Glasgow Haskell Compiler. Now merged into the GHC tree!
https://gitlab.haskell.org/ghc/ghc/tree/master/hadrian
MIT License
208 stars 39 forks source link

set integer-gmp flag when appropriate, when building the compiler lib #684

Closed alpmestan closed 5 years ago

alpmestan commented 5 years ago

This among other things has the effect of giving a correct output for many tests (see RtClosureInspect.hs to see pretty-printing code that's enabled when the integer-gmp flag is enabled (which in turns defines the INTEGER_GMP macro)), but is anyway the correct thing to do.

snowleopard commented 5 years ago

@alpmestan Thank you! I have no idea where stage2 ? arg "integer-simple" came from -- must be an accidental performance tweak that made it through to the master branch.

alpmestan commented 5 years ago

I suspect so yes. I tried notStage0 but we might even want to remove that? I'm not sure. All I know is that this patch improves the resulting compiler (as in, it passes more tests).

snowleopard commented 5 years ago

@alpmestan I think this is the relevant bit from Make build system:

# ----------------------------------------
# Special magic for the integer package

ifneq "$(CLEANING)" "YES"
ifeq "$(INTEGER_LIBRARY)" "integer-gmp"
libraries/base_dist-install_CONFIGURE_OPTS += --flags=integer-gmp
compiler_stage2_CONFIGURE_OPTS += --flags=integer-gmp
else ifeq "$(INTEGER_LIBRARY)" "integer-simple"
libraries/base_dist-install_CONFIGURE_OPTS += --flags=integer-simple
else
$(error Unknown integer library: $(INTEGER_LIBRARY))
endif
endif

Shall we mirror these settings in Hadrian?

If yes, we should do stage2 ? intLib == integerGmp ? arg "integer-gmp", plus check the base setting.

alpmestan commented 5 years ago

@snowleopard Hmm indeed. Or stage1? I get confused.

Do you feel like making this change? Do you want it to happen in this PR or another one? In any case, to make sure that this has the same positive effect that this PR has, you want to make sure that:

$ hadrian/build.sh -j4 --flavour=perf --build-root=_tmp --trace test --only=print002

doesn't cause any failure.

snowleopard commented 5 years ago

Actually, you were right initially: notStage0 is likely what we want. compiler_stage2 corresponds to building Stage2 compiler, and this happens in Stage1. But I think we also want to do the same when building Stage3 compiler (as a self-check), so it seems safer to pass the flag in all stages apart from Stage0.

Let's make the following change in this PR:

-- base
notStage0 ? arg (pkgName intLib)

-- compiler
notStage0 ? intLib == integerGmp ? arg "integer-gmp"

Could the base flag fix some other failing tests by the way?

alpmestan commented 5 years ago

Good question. I'm trying your base suggestion, I'll report back.

alpmestan commented 5 years ago

Looks like it doesn't.

alpmestan commented 5 years ago

Do you want that base bit as well nonetheless, in this PR?

snowleopard commented 5 years ago

Do you want that base bit as well nonetheless, in this PR?

Yes, I think we better do this in this PR. Thank you. I'll merge once CI bots complete.