Added primops 'timesInt2#' and 'mulIntC#'.
Needs RevisionPublic

Authored by clintonmead on Aug 5 2017, 6:56 AM.



I've added two primops: 'timesInt2#' and 'mulIntC#'.

'timesInt2#' is like 'timesWord2#' but performs signed multiplication,
and 'mulIntC#' is like 'addIntC#' and 'subIntC#' but reports signed multiplicative overflow.

I did attempt to use this new primops in 'GHC.Integer.Type' in an attempt to speed up Integer
arithmetic but my success there has been mixed, so I decided just to submit this patch
on it's own.

This is my first GHC patch.

I have defined LLVM, x86 and PowerPC code for both of the new primops, and also CMM code,
although the CMM code for 'timesInt2#' is quite horrific.

I have ran the full test suite with "timesInteger" redefined to use both these primops
and also done some informal testing, however I have omitted the changes to 'GHC.Integer.Type'
I this patch so this primops are currently unused. Hence there probably needs to be some
test cases but I haven't included them yet.

Test Plan

None as yet, needs some test cases. Unsure of how to do this as this is my first GHC patch.

Diff Detail

rGHC Glasgow Haskell Compiler
Lint WarningsExcuse: Lots of lines are longer than 80 chars in this file already.
Warningcompiler/codeGen/StgCmmPrim.hs:1060TXT3Line Too Long
Warningcompiler/codeGen/StgCmmPrim.hs:1105TXT3Line Too Long
Warningcompiler/codeGen/StgCmmPrim.hs:1119TXT3Line Too Long
Warningcompiler/codeGen/StgCmmPrim.hs:1125TXT3Line Too Long
Warningcompiler/codeGen/StgCmmPrim.hs:1129TXT3Line Too Long
Warningcompiler/llvmGen/LlvmCodeGen/CodeGen.hs:563TXT3Line Too Long
Warningcompiler/nativeGen/PPC/CodeGen.hs:1239TXT3Line Too Long
Warningcompiler/nativeGen/PPC/CodeGen.hs:1401TXT3Line Too Long
Warningcompiler/nativeGen/X86/CodeGen.hs:2208TXT3Line Too Long
No Unit Test Coverage
Build Status
Buildable 17051
Build 31927: [GHC] Linux/amd64: Patch building
Build 31926: [GHC] OSX/amd64: Continuous Integration
Build 31925: [GHC] Windows/amd64: Continuous Integration
Build 31924: arc lint + arc unit
clintonmead created this revision.Aug 5 2017, 6:56 AM
clintonmead updated this revision to Diff 13445.Aug 5 2017, 7:03 AM

I got the diff wrong last time, this should be the full diff.

bgamari edited edge metadata.Aug 5 2017, 8:28 PM

Thanks @clintonmead! I'm travelling currently so it may take a bit longer than usual to do a proper review.

That being said, I had a brief look now and this looks great.


Can you turn the comment here into a proper docstring? This should be set between braces after the type signature. See, for instance, subIntC# above. This should be done for all three changed/new primops.

  • Added and updated primops docs

Added some doc strings not only to the two primop I've added but a few others.

Also moved the position of my added primops so they're in the appropriate section (i.e Int or Word).

Oh, I also added commutable to "addIntC#" while I was there. "addIntC#" is not one of the primops I added but presumably it should be marked commutable.

Note that both "addIntC#" and "subIntC#" are marked "code_size = 2". I have no idea what this means. I haven't marked the primop I've added "mulIntC#" as "code_size = 2" because well, I have no idea what it means...

Oh, I also added commutable to "addIntC#" while I was there. "addIntC#" is not one of the primops I added but presumably it should be marked commutable.

Sounds right to me.

Would you like to discuss the testcase?

clintonmead marked an inline comment as done.Aug 18 2017, 4:30 AM
This comment was removed by clintonmead.

Doc Strings have been added.

You will find some documentation here. Essentially you just write a small program emitting some output to stdout, add an entry to a .T file, and record the expected output of the program. If you have any questions feel free to send an email or find me on IRC.

clintonmead marked 2 inline comments as done.Aug 18 2017, 8:01 AM

No problem I'll give it a go this weekend.

bgamari requested changes to this revision.Aug 26 2017, 11:32 PM

Let me know if you would like help.

This revision now requires changes to proceed.Aug 26 2017, 11:32 PM
austin resigned from this revision.Nov 9 2017, 5:36 PM