Hi Mike - They are already separate requests... obviously since they are separate commits. Somebody reviewing one commit should just review that commit.
Without *all* of these fixes, the LLVM backend does not work. That's why I consider it one pull request. The patches can still be reviewed separately. For the changes in "Handle type variables in a few special places" and "Don't heap register type variables" - I don't understand what changes caused this to happen, but now we are getting type variables doing things *in codegen*. Since they are types, I would expect them to be gone by codegen... The code generator tries to generate someTypeVariable = somethingElse. I have no idea why it works in the C backend and I think this kind of thing is in error. I think the LLVM backend just complains more loudly about weird things. - Some of these changes are specific to LLVM (to make sizeof(c_int) work). It used to work (unmodified) and again I havn't done archeology to find out what broke, but it seems that the broke thing needs fixing in any case. - Somebody who understands type variables should review those changes -michael On 08/07/2014 07:52 PM, Mike Noakes wrote: > > On Aug 7, 2014, at 2:44 PM, Michael Ferguson <[email protected]> wrote: > >> Hi - >> >> OK, I have pull request #124 about this, >> but: >> >> I was very surprised to see >> symbols with FLAG_TYPE_VARIABLE ending up all the way >> in codegen. In particular, I'd recommend that somebody >> add an assertion failure near "Why are we getting here?" >> around expr.cpp:3721 in the patched version -- I don't >> think that code should be necessary. >> >> >> Thanks, >> >> -michael > > > Michael, > > [This is direct email rather than GIT-HUB comments]. > > > > I took a look at this pull request but I am not certain I understand the scope > for some of the changes. It seems to me that this request touches more than > one issue but I am not certain how many issues are involved. > > Tom and I took a look at this and we concluded that I should seek some > clarification. > > > 1) The “fix up —about” that you suggest Sung look at seems to be a separable > issue. Is that right? > > It might have been slightly easier if this had been in its own pull-request > so that it > could be reviewed/applied more easily. > > Depending on the scope of the other changes we may be able to handle this on > our side or we might ask for a separate pull-request. > > > 2) There’s a trivial change in getIntermediateDirName() that is easy to > understand > and approve. > > > 3) I understand the change you made in externCResolve. There are two > relatively simple changes for code that is conditional on LLVM. It is easy to > approve > and merge these changes. > > > 4) So finally there are the other two commits. We are unsure if this is an > effort > to fix a different issue that you became aware of or if this is a proposed > solution > for some other consequences of moving the initFun stuff that we haven’t > appreciated. > > A yellow flag for us is that the change in expr.cpp appears to run for both > LLVM and > STD but we don’t know of a problem in STD that this change is trying to > address. > > Could you provide some additional background? > > With thanks and regards, > > Mike > > > > > >> >> On 08/07/2014 01:11 PM, Michael Ferguson wrote: >>> Hi Mike - >>> >>> >>> Thanks very much for your help! >>> >>> module->block->insertAtHead(result) seems to get the job done. I'm doing >>> more testing now, but expect a pull request from me soon fixing this >>> and maybe another LLVM problem. >>> >>> (We really need to get nightly/weekly LLVM testing going somehow...) >>> >>> Best, >>> >>> -michael >>> >>> On 08/07/2014 12:26 PM, Mike Noakes wrote: >>>> >>>> On Aug 7, 2014, at 8:41 AM, Michael Ferguson <[email protected]> wrote: >>>> >>>>> Hi - >>>>> >>>>> I've noticed that >>>>> chpl --print-passes test/extern/ferguson/externblock/define.chpl >>>>> now core dumps in the compiler because >>>>> the extern block support tries to do >>>>> module->initFn->insertAtHead(result); >>>>> when trying to add a global variable, >>>>> but module->initFn is NULL. >>>>> >>>>> This used to work but I recall seeing some recent >>>>> changes to module initialization. >>>>> >>>>> Since readExternC runs fairly early on >>>>> (so that it can happen before scopeResolve), >>>>> it sometimes changes the AST in odd ways. >>>>> How should it add a global variable? >>>>> (Is there a way to request that the module's >>>>> init function be created, for example? Or >>>>> should it be doing something else?) >>>>> >>>>> Thanks, >>>>> >>>>> -michael >>>> >>>> >>>> Hi Michael, >>>> >>>> I did the work to relocate the code that inserts Module Init functions >>>> from Parse >>>> to Normalize. >>>> >>>> I was surprised to see that there is a test that fails but now I see that >>>> it relies on LLVM. >>>> Apparently we haven't done a run with LLVM since I completed that work. >>>> >>>> 1) I'd be happy to take on the work to apply the required changes to >>>> enable the LLVM >>>> build to work with this change. >>>> >>>> 2) Alternatively you might find it is not terribly hard. There is a fair >>>> chance that you >>>> will be able to replace "module->initFn->insertAtHead(result)" with >>>> "module->block->insertAtHead(result)" >>>> >>>> >>>> >>>> By way of a little background: >>>> >>>> Historically the parser collected the statements in the source level code >>>> in to the block >>>> and then ran a final pass in which the AST for a "module init function" is >>>> created and >>>> the most of the original contents of the block were inserted in to the >>>> body of that function >>>> (there are a few exceptions). >>>> >>>> During Normalize, most of body of the init function was pulled back out to >>>> the Module >>>> level leaving only the code necessary to initialize the module-level >>>> variables etc. >>>> >>>> The passes that ran between Parse and Normalize had to "be aware" that >>>> most of the >>>> module was buried inside inside the Module init function; as the function >>>> you are considering >>>> appears to do. >>>> >>>> >>>> >>>> If you are willing I'd like to propose that you take the first step at the >>>> proposed change but >>>> I will be very happy to take over if it is not a relatively simple change. >>>> >>>> With regards, >>>> >>>> Mike >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>>> >>>>> ------------------------------------------------------------------------------ >>>>> Infragistics Professional >>>>> Build stunning WinForms apps today! >>>>> Reboot your WinForms applications with our WinForms controls. >>>>> Build a bridge from your legacy apps to the future. >>>>> http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk >>>>> _______________________________________________ >>>>> Chapel-developers mailing list >>>>> [email protected] >>>>> https://lists.sourceforge.net/lists/listinfo/chapel-developers >>>> >>> >>> >>> ------------------------------------------------------------------------------ >>> Infragistics Professional >>> Build stunning WinForms apps today! >>> Reboot your WinForms applications with our WinForms controls. >>> Build a bridge from your legacy apps to the future. >>> http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk >>> _______________________________________________ >>> Chapel-developers mailing list >>> [email protected] >>> https://lists.sourceforge.net/lists/listinfo/chapel-developers >>> >> >> >> ------------------------------------------------------------------------------ >> Infragistics Professional >> Build stunning WinForms apps today! >> Reboot your WinForms applications with our WinForms controls. >> Build a bridge from your legacy apps to the future. >> http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk >> _______________________________________________ >> Chapel-developers mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/chapel-developers > ------------------------------------------------------------------------------ Want fast and easy access to all the code in your enterprise? Index and search up to 200,000 lines of code with a free copy of Black Duck Code Sight - the same software that powers the world's largest code search on Ohloh, the Black Duck Open Hub! Try it now. http://p.sf.net/sfu/bds _______________________________________________ Chapel-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/chapel-developers
