rts: Replace `nat` with `uint32_t`
ClosedPublic

Authored by erikd on May 1 2016, 11:03 PM.

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.
erikd updated this revision to Diff 7444.May 1 2016, 11:03 PM
erikd retitled this revision from to rts: Replace `nat` with `uint32_t`.
erikd updated this object.
erikd edited the test plan for this revision. (Show Details)
erikd added reviewers: simonmar, thomie, hvr, hsyl20.
erikd planned changes to this revision.May 1 2016, 11:55 PM

Going to fix the line length and whitespace issues.

erikd updated this revision to Diff 7445.May 2 2016, 3:10 AM
erikd edited edge metadata.

Whitespace and line length fixes.

erikd updated this revision to Diff 7453.May 2 2016, 6:19 PM
erikd edited edge metadata.

Rebase against master

simonmar accepted this revision.May 3 2016, 3:35 AM
simonmar edited edge metadata.

Sure, why not.

Might want to sanity-check that this generates the same code as before.

This revision is now accepted and ready to land.May 3 2016, 3:35 AM
erikd added a comment.May 3 2016, 4:00 AM

Yep, checking that it produces identical object code is a good idea. Will do that!

erikd added a comment.May 3 2016, 5:11 AM

Ok did the following:

  • Built the code without the patch.
  • Moved rts/dist/build to build-before
  • Build the code with the patch.
  • Changed into build-before
  • Ran:
for file in *o ; do
    objdump -d $file > ${file}-a.txt
    objdump -d ../rts/dist/build/$file > ${file}-b.txt
    wc -l ${file}-a.txt ${file}-b.txt
    done

and the sizes were identical. There are assembler differences for some of the GhcRTSWays. The *.o, *.p_o, *.l_o ways were all identical, but the others had what seems to changes in constants like:

--- Task.debug_o-a.txt  2016-05-03 20:01:12.496125456 +1000
+++ Task.debug_o-b.txt  2016-05-03 20:01:12.500125374 +1000
@@ -413,7 +413,7 @@
  58d:  e8 6e fa ff ff          callq  0 <myTask>
  592:  48 3b 45 f8             cmp    -0x8(%rbp),%rax
  596:  74 0f                   je     5a7 <boundTaskExiting+0x26>
- 598:  be 41 01 00 00          mov    $0x141,%esi
+ 598:  be 42 01 00 00          mov    $0x142,%esi
  59d:  bf 00 00 00 00          mov    $0x0,%edi
  5a2:  e8 00 00 00 00          callq  5a7 <boundTaskExiting+0x26>
  5a7:  48 8b 45 f8             mov    -0x8(%rbp),%rax

I think these changes are benign.

Ok, strange, I'm not sure what that is, but I agree it's probably benign.

Hmm, although if there are changes in *.thr_o we should peer at those a bit closer. Can you paste a larger diff?

bgamari requested changes to this revision.May 3 2016, 3:56 PM
bgamari edited edge metadata.

Bumping out of review queue while this is investigated.

This revision now requires changes to proceed.May 3 2016, 3:56 PM
hsyl20 edited edge metadata.May 3 2016, 4:38 PM

The constants (like 0x141 in the given excerpt) are just line numbers (cf ASSERT definition).

rts/ProfHeap.h
13–14

Bad alignment

rts/RaiseAsync.c
876

Useless cast

927

useless cast

rts/RetainerSet.c
144

Bad comment alignment

rts/RetainerSet.h
66

bad comment alignment

The constants (like 0x141 in the given excerpt) are just line numbers (cf ASSERT definition).

Of course! But that only accounts for the differences in debug ways, not for differences in *.thr_o.

erikd added a comment.EditedMay 4 2016, 5:10 AM

I just rechecked, the *.thr_o are also identical before and after application of my patch. The only files that are different before and after are the object files which were compiled in debug mode.

erikd updated this revision to Diff 7456.May 4 2016, 5:17 AM
erikd edited edge metadata.
  • Fix issues raised by @hsyl20.
  • Rebase against master.

If nat is at least 32bits and has no other restrictions, shouldn't it be converted to uint_fast32_t rather than uint32_t?

We could have a long debate about this, but personally I think moving to a type that is consistent across platforms is a step in the right direction.

This revision was automatically updated to reflect the committed changes.