Testsuite: add CmmSwitchTest for 32-bit platforms
ClosedPublic

Authored by avd on May 16 2016, 7:11 AM.

Details

Summary

Move CmmSwitchTest to CmmSwitchTest64, because it's broken on 32-bit platforms.
Create CmmSwitchTest32 that repeats CmmSwitchTest64 for platforms with 32-bit
wordsize.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
avd retitled this revision from to Testsuite: add CmmSwitchTest for 32-bit platforms.May 16 2016, 7:11 AM
avd edited the test plan for this revision. (Show Details)
avd updated the Trac tickets for this revision.
avd updated this object.
bgamari accepted this revision.May 16 2016, 7:27 AM

Thanks for doing this, @avd! This looks good to me.

This revision is now accepted and ready to land.May 16 2016, 7:27 AM

Did you make changes to CmmSwitchTestGen.hs as well?

thomie requested changes to this revision.May 16 2016, 7:30 AM
thomie added a reviewer: thomie.

Either CmmSwitchTestGen.hs should get deleted, or it should get updated to also generate CmmSwitchTest32 somehow.

This revision now requires changes to proceed.May 16 2016, 7:30 AM
thomie added a subscriber: nomeata.

Adding @nomeata, as he created this test.

Ahh, I had forgotten that these are generated. Thanks @thomie!

avd added a comment.EditedMay 16 2016, 11:43 AM
In D2226#64370, @thomie wrote:

Either CmmSwitchTestGen.hs should get deleted, or it should get updated to also generate CmmSwitchTest32 somehow.

Neither should be done. CmmSwitchTestGen.hs successfully generates 32-bit version, because it relies on minBound/maxBound for min/max values. In 32-bit environments it generates correct values.

Ahh, I had forgotten that these are generated. Thanks @thomie!

I didn't know that and made it by hand :-( Now I understand why it has lint issues. Anyway, my manual labor version is the same as generated, but I would like to refactor it, to make it more readable. More specifically I want to split long lines and print numbers in hex. Does it make sense?

In D2226#64470, @avd wrote:
In D2226#64370, @thomie wrote:

Either CmmSwitchTestGen.hs should get deleted, or it should get updated to also generate CmmSwitchTest32 somehow.

Neither should be done. CmmSwitchTestGen.hs successfully generates 32-bit version, because it relies on minBound/maxBound for min/max values. In 32-bit environments it generates correct values.

Ahh, I had forgotten that these are generated. Thanks @thomie!

I didn't know that and made it by hand :-( Now I understand why it has lint issues. Anyway, my manual labor version is the same as generated, but I would like to refactor it, to make it more readable. More specifically I want to split long lines and print numbers in hex. Does it make sense?

Sounds reasonable to me. While you are at it perhaps add a comment to the generated output indicating that it was produced by CmmSwitchTestGen.

I didn't know that and made it by hand :-(

CmmSwitchTestGen.hs writes out a warning about that; but it looks like the committed version of the test case was produced before that warning was added – sorry.

Neither should be done. CmmSwitchTestGen.hs successfully generates 32-bit version, because it relies on minBound/maxBound for min/max values. In 32-bit environments it generates correct values.

It could be using fromIntegral (maxBound :: Word64) resp. fromIntegral (maxBound :: Word32) depending on an option. It would be good if the generator could generate both test cases independent of the host architecture, so when one wants to add more checks, anyone can generate them.

avd added a comment.May 17 2016, 4:43 AM

I didn't know that and made it by hand :-(

CmmSwitchTestGen.hs writes out a warning about that; but it looks like the committed version of the test case was produced before that warning was added – sorry.

Neither should be done. CmmSwitchTestGen.hs successfully generates 32-bit version, because it relies on minBound/maxBound for min/max values. In 32-bit environments it generates correct values.

It could be using fromIntegral (maxBound :: Word64) resp. fromIntegral (maxBound :: Word32) depending on an option. It would be good if the generator could generate both test cases independent of the host architecture, so when one wants to add more checks, anyone can generate them.

Ok, good. I'll try to update CmmSwitchTestGen by adding CLI argument to it for choosing 32/64 option and regenerate both tests. This will discard my previous commits, so I would like to rebase and force push new commits. Is that OK? How Phabricator will handle it?

@nomeata, silly question: why do we even check in the generated testcases? Why not simply generate the testcase when the test is run using the host's native word size? This could be easily done with in a make-driven test

@nomeata, silly question: why do we even check in the generated testcases? Why not simply generate the testcase when the test is run using the host's native word size? This could be easily done with in a make-driven test

Hmm, that is a fair idea. I guess I usually try to avoid make-driven tests as much as possible... but maybe it would be fine here.

avd updated this revision to Diff 7638.May 18 2016, 1:30 PM

Update CmmSwitchTestGen to generate 32/64 bit versions

  • Testsuite: Add 32/64 bits support to CmmSwitchTestGen
  • Testsuite: Add generated CmmSwitchTest32/64 versions
  • Testsuite: fix formatting in output of CmmSwitchTestGen
  • Testsuite: regenerate CmmSwitchTest32/64 with format fix
nomeata added inline comments.May 18 2016, 4:44 PM
testsuite/tests/codeGen/should_run/CmmSwitchTestGen.hs
84

I’d say: Fail here, (using error is ok), and remove Unknown from the data type; since you use if-then-else, Unknown would silently be treated as 64 bit, which might confuse someone who uses the program without reading the code.

avd updated this revision to Diff 7654.May 19 2016, 1:47 AM

Require -32/-64 option for CmmSwitchTestGen

avd marked an inline comment as done.May 19 2016, 2:04 AM

@nomeata @bgamari I also wanted to rebase my commits to reorder and squash them. Is there any special arcanist workflow or should I just arc diff --update D2226 <branch root>?

Nothing in particular, just use the appropriate base commit and the arcanist will in effect squash the commits.

nomeata accepted this revision.May 19 2016, 2:55 AM

If I’m reading Phabricator correclty, your latest iteration lost the updated test case code, but otherwise this LGTM. Thanks!

avd added a comment.May 19 2016, 3:50 AM

If I’m reading Phabricator correclty, your latest iteration lost the updated test case code

Do you mean al_check function? This one had duplicated test cases not generated by CmmSwitchTestGen but somehow committed into tree. So with the last regeneration it was vanished, no worries.

avd updated this revision to Diff 7655.May 19 2016, 5:02 AM

Rebased commits to cleanup history (squashed and reworded)

  • CmmSwitchTestGen 32/64 bits version
  • Add generated CmmSwitchTest32/64 (fixes Trac #11297)
bgamari accepted this revision.May 19 2016, 7:43 AM

Looks pretty reasonable to me. I still wonder whether we should just make this a make driven test and generate the test module on the fly, but this is fine for now.

austin accepted this revision.May 19 2016, 5:23 PM

Minor wibble, but not a big deal. LGTM.

testsuite/tests/codeGen/should_run/CmmSwitchTestGen.hs
67

If you wouldn't mind, could you add this to read:

"-- This file is generated from CmmSwitchTestGen! @generated"

This will make arc stop getting angry about lint errors in the future and will also make this diff substantially smaller. I may go through and see if any other files are generated and fix this.

It would be nice if we could document this convention somewhere. This is
the first time I've heard of @generated.

avd updated this revision to Diff 7668.May 20 2016, 5:11 AM

Add "@generated" to the generated tests to please arcanist

avd marked an inline comment as done.May 20 2016, 9:33 AM

@austin Is it ready to land?

testsuite/tests/codeGen/should_run/CmmSwitchTestGen.hs
67

Sure, will prepare another diff shortly.

This revision was automatically updated to reflect the committed changes.