Hello,I would appreciate some help in debugging a Cmm Lint error, I have been stuck on for quite a while.
Basically I am adding support for Int32# on top of the In8#(D4475) and Int16#(D5006) patches.The Cmm being generated for the test programs are incorrect.Taking a sample test like this (https://github.com/Abhiroop/ghc-1/blob/int8/testsuite/tests/ffi/should_run/PrimFFIInt32.hs)The crux of the linting error is the type mismatch between LHS and RHS of the CmmAssign statement.For example in the Int8 patch, the Cmm generated looks like this:```_s1MC::I8 = I8[Sp]; // CmmAssign_s1MD::I8 = I8[Sp + 8]; // CmmAssign_s1ME::I8 = I8[Sp + 16]; // CmmAssign_s1MF::I8 = I8[Sp + 24]; // CmmAssign_s1MG::I8 = I8[Sp + 32]; // CmmAssign_s1MH::I8 = I8[Sp + 40]; // CmmAssign_s1MI::I8 = I8[Sp + 48]; // CmmAssign_s1MJ::I8 = I8[Sp + 56]; // CmmAssign_s1MK::I8 = I8[Sp + 64]; // CmmAssign_s1ML::I8 = I8[Sp + 72]; // CmmAssignR6 = %MO_XX_Conv_W8_W64(_s1MG::I8); // CmmAssignR5 = %MO_XX_Conv_W8_W64(_s1MF::I8); // CmmAssignR4 = %MO_XX_Conv_W8_W64(_s1ME::I8); // CmmAssignR3 = %MO_XX_Conv_W8_W64(_s1MD::I8); // CmmAssignR2 = %MO_XX_Conv_W8_W64(_s1MC::I8); // CmmAssign```or```_c1Oq::I8 = %MO_SS_Conv_W64_W8(9); // CmmAssign_s1N0::I8 = _c1Oq::I8; // CmmAssign_c1Ou::I8 = %MO_SS_Conv_W64_W8(8); // CmmAssign_s1MZ::I8 = _c1Ou::I8; // CmmAssign_c1Ox::I8 = %MO_SS_Conv_W64_W8(7); // CmmAssign_s1MY::I8 = _c1Ox::I8; // CmmAssign_c1OA::I8 = %MO_SS_Conv_W64_W8(6); // CmmAssign_s1MX::I8 = _c1OA::I8; // CmmAssign_c1OD::I8 = %MO_SS_Conv_W64_W8(5); // CmmAssign_s1MW::I8 = _c1OD::I8; // CmmAssign_c1OG::I8 = %MO_SS_Conv_W64_W8(4); // CmmAssign_s1MV::I8 = _c1OG::I8; // CmmAssign_c1OJ::I8 = %MO_SS_Conv_W64_W8(3); // CmmAssign_s1MU::I8 = _c1OJ::I8; // CmmAssign```Basically the registers on the left hand side have type `I8`, whereas in my case the registers on the LHS always have the width equal to the word size:```_c1Ok::I64 = %MO_SS_Conv_W64_W32(9); // CmmAssign_s1N0::I64 = _c1Ok::I64; // CmmAssign_c1Oo::I64 = %MO_SS_Conv_W64_W32(8); // CmmAssign_s1MZ::I64 = _c1Oo::I64; // CmmAssign_c1Or::I64 = %MO_SS_Conv_W64_W32(7); // CmmAssign_s1MY::I64 = _c1Or::I64; // CmmAssign_c1Ou::I64 = %MO_SS_Conv_W64_W32(6); // CmmAssign_s1MX::I64 = _c1Ou::I64; // CmmAssign_c1Ox::I64 = %MO_SS_Conv_W64_W32(5); // CmmAssign_s1MW::I64 = _c1Ox::I64; // CmmAssign_c1OA::I64 = %MO_SS_Conv_W64_W32(4); // CmmAssign_s1MV::I64 = _c1OA::I64; // CmmAssign_c1OD::I64 = %MO_SS_Conv_W64_W32(3); // CmmAssign_s1MU::I64 = _c1OD::I64; // CmmAssign_c1OG::I64 = %MO_SS_Conv_W64_W32(2); // CmmAssign_s1MT::I64 = _c1OG::I64; // CmmAssign```
I would ideally want to have the datatype of the LHS be `I32`.
Michal, I thought this type is picked up using the `primRepCmmType` function https://github.com/Abhiroop/ghc-1/blob/int8/compiler/cmm/CmmUtils.hs#L104-L105 which I modified but I am not sure why the LHS types of the CmmAssign statement allocate `I64` instead of `I32`. Is there any other change on your patch(D4475) which I might have overlooked.I have uploaded my Int32# patch on phab as well for reference(https://phabricator.haskell.org/D5032)
Thanks,Abhiroop
On Wed, Aug 1, 2018 at 3:45 PM Michal Terepeta <michal.terepeta@gmail.com> wrote:I've rebased the diff and relaxed the assertion - do take a look if that looks reasonable to you :)On Wed, Jul 25, 2018 at 9:03 PM Michal Terepeta <michal.terepeta@gmail.com> wrote:Hi Carter,I didn't write this assertion. I only validated locally (IIRC at the time I uploaded the diff, harbormaster was failing for some other reasons).I'll try to have a look at it this weekend.Cheers!- MichalOn Wed, Jul 25, 2018 at 2:16 AM Carter Schonwald <carter.schonwald@gmail.com> wrote:Michal: did you write this Assert about width? and if so could you explain it so we can understand?hrmm... that code is in the procedure for generating C calls for 64bit intel systemshttps://github.com/michalt/ghc/blob/int8/compiler/nativeGen/X86/CodeGen.hs#L2541 is the top of that routineand width is defined right below the spot in questionit seems like the code/assertion likely predates michal's work? (eg, a trick to make sure that genCCall64 doesn't get invoked by 32bit platforms?)On Mon, Jul 23, 2018 at 8:54 PM Abhiroop Sarkar <asiamgenius@gmail.com> wrote:Hi Michal,In the tests that you have added to D4475, are all the tests running fine?On my machine, I was running the FFI tests(https://github.com/michalt/ghc/blob/int8/testsuite/tests/ffi/should_run/PrimFFIInt8.hs) and they seem to fail at a particular assert statement in the code generator.To be precise this one: https://github.com/michalt/ghc/blob/int8/compiler/nativeGen/X86/CodeGen.hs#L2764Upon commenting that assert the tests run fine. Am I missing something or is the failure expected?Thanks,AbhiroopOn Mon, Jul 9, 2018 at 8:31 PM Michal Terepeta <michal.terepeta@gmail.com> wrote:Just for the record, I've uploaded the changes to binary:- MichalOn Wed, Jul 4, 2018 at 11:07 AM Michal Terepeta <michal.terepeta@gmail.com> wrote:Yeah, if you look at the linked diff, there are a few tiny changes to binary that are necessary.I'll try to upload them to github later this week.Ben, is something blocking the review of the diff? I think I addressed all comments so far.- MichalOn Wed, Jul 4, 2018 at 1:38 AM Abhiroop Sarkar <asiamgenius@gmail.com> wrote:Hello Michal,I was looking at your diff https://phabricator.haskell.org/D4475 and there seems to be some changes that you perhaps made in the binary package (https://phabricator.haskell.org/differential/changeset/?ref=199152&whitespace=ignore-most). I could not find your version of binary on your github repos list. Is it possible for you to upload that so I can pull those changes?Thanks
AbhiroopOn Mon, May 28, 2018 at 10:45 PM Carter Schonwald <carter.schonwald@gmail.com> wrote:Abhiroop has the gist, though the size of word args for simd is more something we want to be consistent between 32/64 bit modes aka ghc targeting 32 or 64 bit intel with simd support :). It would be best if the unpack and pack operations that map to and from unboxed tuples where consistent and precise type wise.-CarterHello Michal,My understanding of the issues are much lesser compared to Carter. However, I will state whatever I understood from discussions with him. Be warned my understanding might be wrong and Carter might be asking this for some completely different reason.> Out of curiosity, why do you need Word64#/Word32# story to be fixed for SIMD?One of the issues we are dealing with is multiple microarchitectures(SSE, AVX, AVX2 etc). As a result different microarchitectures has different ways of handling an 8 bit or 16 bit or 32 bit unsigned integer. An example I can provide is the vector multiply instruction which has different semantics of multiplying and storing the first 16 bits of a 32 bit unsigned integer compared to the lower 16 bits for each of the elements in its SIMD registers. There will be some other intricacies around handling a byte sized integer or a 64 bit integer which will be different from the 32 bit version.Basically a 128 bit vector instruction will make some minor differences when dealing with 2 64 bit integers or 4 32 bit integer or 8 16 bit integers.So I think at the higher level we would want to be as precise as possible when specifying the datatypes and want things like Word8#, Word16#, Word32#, Word64# rather than a single ambiguous type like Word.One more example is this vector operation like : broadcastWord64X2# :: Word# -> Word64X2# which should rather be broadcastWord64X2# :: Word64# -> Word64X2#Another reason could be improving the space efficiency of packing various datatypes. This was explained in some detail in this comment: https://github.com/ghc-proposals/ghc-proposals/pull/74#issuecomment-333951559Thanks,AbhiroopOn Mon, May 28, 2018 at 6:06 PM, Michal Terepeta <michal.terepeta@gmail.com> wrote:Hey Carter,Yeah, I'm totally snowed under with personal stuff at the moment, so not much time to hack onanything. I've managed to make some progress on https://phabricator.haskell.org/D4475 (prototypethat adds Int8#/Word8#; waiting for another round of review). Sadly June still looks a bit crazy forme, so I can't promise anything for Word64#/Word32# (and Int64#/Int32#). I hope to have some moretime in July.If this is blocking you, then please don't wait for me and go ahead (my diff for Int8#/Word8# mightbe a decent starting point to see which places might need changes).Out of curiosity, why do you need Word64#/Word32# story to be fixed for SIMD? Cmm already hasseparate types for vector types (see TyCon.hs and VecRep constructor of PrimRep).Cheers!- MichalPS. Are you coming to ZuriHac by any chance?On Mon, May 28, 2018 at 12:37 AM Carter Schonwald <carter.schonwald@gmail.com> wrote:Hey Michal!So i'm mentoring Abhiroop around getting SIMD all nice and awesome, and I realized we still dont quite yet have the nice Word32# and Word64# etc story in ghc as yet.Whats the status on that and or is there a way we can help get that over the hump for ghc? These different sized in register ints and friends have a tight partnership with any nice SIMD iterating over the summer, and hopefully helps inform the design-Carter--Kloona - Coming Soon!--Kloona - Coming Soon!--Kloona - Coming Soon!--Kloona - Coming Soon!