Hi - To be clear, the changes we discussed have been committed, but now there is some special handling for FLAG_EXTERN + FLAG_TYPE_SYMBOL in the LLVM backend in order to handle this business with sizeof.
I continue to have the position that the code generator should not need to know about type symbols; something is either a Type or a Variable... Yes, sizeof(c_int) passes a VarSymbol with FLAG_TYPE_SYMBOL as you guessed. -michael On 08/25/2014 01:42 PM, Brad Chamberlain wrote: > > I was hoping someone else would wrap up this thread before I got back from > vacation. No such luck... :) > > I'm still not going to be able to answer your overarching question without > more effort, but chiming in on the can of worms that I opened: > > Generally speaking, I think you're right that type symbols shouldn't have > much presence in the generated code, at least for standard types like ints, > because they should be resolved statically. Offhand, I don't know the answer > to whether types with a runtime component (like arrays) have a greater need > to preserve the type through to codegen time -- I'd need to think about that > more. > > I do know that there is a capability to have an extern procedure take a type > argument, such that sizeof() can be prototyped, for example. And for this to > work, presumably the type's symbol would need to be code-generated. I used > this trick in your util/config/makeSysBasicTypes file, for example, to ensure > that we got the sizes correct for all of the c_* type aliases. > > If I weren't catching up on two weeks of email, I'd take a look at the IR to > see if the expressions passed to the sizeof() calls in the resulting > gen/*/SysCTypes.chpl module (like 'c_int' in 'sizeof(c_int)') are VarSymbols > with FLAG_TYPE_SYMBOL attached. From your mail, I'd guess they are. Whether > or not they should be is a reasonable question. > > -Brad > > > On Tue, 12 Aug 2014, [email protected] wrote: > >> Brad - >> >> Yes, right now >> extern type c_int = int(32); >> >> results in c_int being a VarSymbol with FLAG_TYPE_VARIABLE. >> >> That doesn't really make sense to me though since by the time we get to code >> generation, what should the code generator do with FLAG_TYPE_VARIABLE >> VarSymbols? Are these always types? (or sometimes "runtime types" which are >> actually values)? >> >> If they're always types, there's not really any point in doing the usual >> variable things... e.g. allocating on the stack or assigning to them ... but >> I saw the code generator generating code that assigns to a VarSymbol with >> FLAG_TYPE_VARIABLE, and turning it off makes arrays of arrays no longer >> work... so the whole situation doesn't make any sense to me. >> >> But, it would help me (and maybe others) if somebody would clarify what >> VarSymbol + FLAG_TYPE_VARIABLE is used for (vs just being a type) and what >> the code generator should do with such things. Is it sometimes a runtime >> type and sometimes not? If so, is there a way we can or should distinguish >> between those two cases? >> >> Thanks, >> >> -michael >> >> ________________________________________ >> From: Brad Chamberlain [[email protected]] >> Sent: Friday, August 08, 2014 18:32 >> To: [email protected] >> Cc: [email protected] >> Subject: Re: [Chapel-developers] module init functions >> >> I haven't been following this closely, but Michael's FLAG* question jumped >> out at me. Wouldn't: >> >> Chapel: >> >> extern type age = c_int; >> >> where C's header said: >> >> typedef int age; >> >> result in Chapel's notion of 'age' having both FLAG_TYPE_VARIABLE and >> FLAG_EXTERN on it? >> >> -Brad >> >> >> >> On Fri, 8 Aug 2014, Michael Ferguson wrote: >> >>> Hi - >>> >>> See my pull request - I've backed out the changes for FLAG_TYPE_VARIABLE >>> VarSymbols, so now we just need Sung (or somebody) to review the --about >>> changes. >>> >>> I still don't understand why FLAG_TYPE_VARIABLE + FLAG_EXTERN makes any >>> sense >>> though.... >>> >>> -michael >>> >>> On 08/08/2014 12:36 PM, Michael Ferguson wrote: >>>> 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 >>>> >>> >>> >>> ------------------------------------------------------------------------------ >>> 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 >>> ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ Chapel-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/chapel-developers
