| Message ID | CANeU7QnLE+kH=mrdy_u42oPFwgRYzy_EVc=SWZ4OVtGt2V_N-g@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sat, Feb 24, 2018 at 3:30 PM, Christopher Li <sparse@chrisli.org> wrote: > On Sat, Feb 24, 2018 at 2:00 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: >> Hi Chris, >> >> I saw that a few patches have been pushed on the head, >> including the two I disagreed with and which I explained *why* >> they where wrong. Well. >> >> Meanwhile, I see some regressions with the current head. >> For example some simple programs like: >> void a(void) >> { >> int b[] = { 8 }; >> int c; >> for (;;) >> b[c] = b[0]; >> } >> now crashes when used with test-linearize. > > Oops, reproduced. Thanks for the report. > > I see pseudo->ident polluted by pseudo->size. > Separate them out of the union seems to fix the crash. Right, that is the exactly the course. I found the place that use the ident without limit to PSEUDO_VAL. found_dominator: br = delete_last_instruction(&parent->insns); phi = alloc_phi(parent, one->target, one->size); phi->ident = phi->ident ? : one->target->ident; <================= add_instruction(&parent->insns, br); use_pseudo(insn, phi, add_pseudo(dominators, phi)); } END_FOR_EACH_PTR(parent); So separate the "ident" and "size" in the union should fix this crash. A little bit too aggressive on saving space. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 24, 2018 at 03:30:44PM -0800, Christopher Li wrote: > On Sat, Feb 24, 2018 at 2:00 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > Hi Chris, > > > > I saw that a few patches have been pushed on the head, > > including the two I disagreed with and which I explained *why* > > they where wrong. Well. > > > > Meanwhile, I see some regressions with the current head. > > For example some simple programs like: > > void a(void) > > { > > int b[] = { 8 }; > > int c; > > for (;;) > > b[c] = b[0]; > > } > > now crashes when used with test-linearize. > > Oops, reproduced. Thanks for the report. > > I see pseudo->ident polluted by pseudo->size. > Separate them out of the union seems to fix the crash. The right and wise thing would have to: 1) take the 2 patches that Jason is waiting for since 5 months (if you agree with). 2) take Randy's patch with the early return NULL 3) take Randy's patch to silence the indirect_branch attribute 4) take my patch to disable the default -Wunknown-attribute 5) inc the version number 6) push a release so that people stop seeing the current 148K+ warnings about the unknown indirect_branch attribute. But people are still waiting and seeing these 148K+ useless warnings and you have forced in a patch which: 1) I explained to you why it was wrong 2) nobody need (even not Dibyendu who, since many weeks, has taken what he wanted in hiw heavily modified tree) 3) have not been tested (I told you that *if* you make changes to the generated code, you better should test it. You have zero such tests) 4) contains a beginner error Now you still have the choice to Do The Right Thing: revert these two patches and push this release or display the level your stubbornness can reach. What will you do? -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 25, 2018 at 1:25 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > > The right and wise thing would have to: > 1) take the 2 patches that Jason is waiting for since 5 months > (if you agree with). I think I have apply them a few days ago. Unless it is a different set. > 2) take Randy's patch with the early return NULL > 3) take Randy's patch to silence the indirect_branch attribute Already in the tip. > 4) take my patch to disable the default -Wunknown-attribute Sure. I am about to ask this. It has been puzzling me for a while. You did not send git merge request any more. Per our agreement I can't directly apply your patch. I did not have a chance to comment on every one of your patch (some did not apply to my tree). But there is a lot of patches I did comment on looks good did not come back as git merges. Come to think of it. Almost none. > 5) inc the version number > 6) push a release so that people stop seeing the current > 148K+ warnings about the unknown indirect_branch attribute. > > But people are still waiting and seeing these 148K+ useless > warnings and you have forced in a patch which: > 1) I explained to you why it was wrong I have look back to your email. I have answer your concerns. Did you write this in the last email I did reply on " *eventually* it may very well be the right solution" ========== quote from your email =========== > > Now, I have no hard problem with the PSEUDO_VAL.size > approach from a theorical point of view (even if I would > prefer the mathematical PoV that 0 is 0, 1 is 1, etc.). > I even think that *eventually* it may very well be > the right solution (probably the whole typing at IR > level need to be revisited). I think constant pseudo have a size is the right solution in the long run. =============== end quote============ The V2 patch has send out in Nov to address your concern that it does not have end to end testing. Dibyendu Majumdar was busy and he eventual test it come back OK. You are CC on the whole thread and no other email on that thread. It is 4 month ago. > 2) nobody need (even not Dibyendu who, since many weeks, has > taken what he wanted in hiw heavily modified tree) Dibyendu did care enough to test it. Thanks him for the effort. > 3) have not been tested (I told you that *if* you make changes > to the generated code, you better should test it. You have > zero such tests) As you know I did have the full kernel compile test on it. It give the same result. So it is not badly affecting the kernel checking for sure. > 4) contains a beginner error Do you know if that follow up patch can fix it? If that is the eventually right direction, it has bug not as intended, then we fix the bug. If it has directional issue, then that is a different story. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Luc, On 25 February 2018 at 09:25, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > The right and wise thing would have to: > 1) take the 2 patches that Jason is waiting for since 5 months > (if you agree with). > 2) take Randy's patch with the early return NULL > 3) take Randy's patch to silence the indirect_branch attribute > 4) take my patch to disable the default -Wunknown-attribute > 5) inc the version number > 6) push a release so that people stop seeing the current > 148K+ warnings about the unknown indirect_branch attribute. > > But people are still waiting and seeing these 148K+ useless > warnings and you have forced in a patch which: I am not sure why above is the 'right thing'. There does not appear to be a definition or process to identify fixes or changes that people need urgently - and if I recall correctly you previously objected to having such a process when I suggested it. > 1) I explained to you why it was wrong Is it wrong because you disagree with it? I don't see a problem with the approach. I tested your approach of OP_PUSH as well - and in my view after comparing the two, this is more correct as it avoids creating a pseudo IR instruction. > 2) nobody need (even not Dibyendu who, since many weeks, has > taken what he wanted in hiw heavily modified tree) Actually I do want this patch because it is blocking the other LLVM patches which would benefit Sparse. > 3) have not been tested (I told you that *if* you make changes > to the generated code, you better should test it. You have > zero such tests) > 4) contains a beginner error > > > Now you still have the choice to Do The Right Thing: > revert these two patches and push this release or display the > level your stubbornness can reach. > What will you do? > I wish the arguments were more technical and less confrontational. As I mentioned above, I am not sure who defines the 'Right Thing'. Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 25, 2018 at 03:44:01AM -0800, Christopher Li wrote: > > I am about to ask this. It has been puzzling me > for a while. You did not send git merge request any more. > Per our agreement I can't directly apply your patch. Sigh. Don't be puzzled. I told you in November that I considered our agreement as finished [1]. You even replied to it. I have indeed stopped to send you pull requests since then. I've stopped to waste my time to send you series and then having to wait for months to have some feedback about some stylistic or other surface issues (and rarely more) and then having again to wait for months that you take the PR. If you want to use some of the patches I send on the ML, please use patchwork or whatever suits you the best. [1] https://marc.info/?l=linux-sparse&m=151052797925158&w=4 > But there is a lot of patches I did comment on looks good A lot? Really? Isn't that more like 2 or 3 very small series among the simplest ones? > > But people are still waiting and seeing these 148K+ useless > > warnings and you have forced in a patch which: > > 1) I explained to you why it was wrong > > I have look back to your email. I have answer your concerns. > > Did you write this in the last email I did reply on > " *eventually* it may very well be the right solution" > > ========== quote from your email =========== > > > > Now, I have no hard problem with the PSEUDO_VAL.size > > approach from a theorical point of view (even if I would > > prefer the mathematical PoV that 0 is 0, 1 is 1, etc.). > > I even think that *eventually* it may very well be > > the right solution (probably the whole typing at IR > > level need to be revisited). > > I think constant pseudo have a size is the right > solution in the long run. > =============== end quote============ > > The V2 patch has send out in Nov to address your > concern that it does not have end to end testing. > Dibyendu Majumdar was busy and he eventual > test it come back OK. > You are CC on the whole thread and no other > email on that thread. It is 4 month ago. Absolutely, I wrote that. Note the 'may', the 'eventually', the 'probaly'. Remember the 'longer term' and some 'details' about CSE and casts (not quoted here). I didn't replied because your V2 didn't address any of the issues. It was the same solution with the same deep problems. There was need for a test to know that. I knew that, you knew that too. Plus, you didn't did any testing I advised to do (at IR level). Didn't you understood that if I advised to do these sort of testing it was because the discussion was sterile and that I knew it would be the only way you could be convinced that there was some issues: seeing them with your own eyes? > > 2) nobody need (even not Dibyendu who, since many weeks, has > > taken what he wanted in hiw heavily modified tree) > > Dibyendu did care enough to test it. Thanks him for the effort. I care enough to have taken a lot of my time (to try) to explain you why the approach was wrong given the current situation. Needless to say, I failed. Testing is very very valuable. And I'm very grateful to people who take the time to test my things. However, sorry for the truism, no testing can prove the absence of defects in your code, it can only show you their presence. Hence the importance of thinking first about the code and understand it, having a good test coverage, different kind of testing. Hence my insistence that, *if* you make changes to the generated IR, you *need* to have good ways of testing at IR level. Also, testing is the final step, when you can maybe find some bugs but after the principles have already been validated. In short: testing doesn't dispense you to think about the code. > > 3) have not been tested (I told you that *if* you make changes > > to the generated code, you better should test it. You have > > zero such tests) > > As you know I did have the full kernel compile test on it. It give > the same result. A kernel compile is essentially useless to test the generated IR. You need specific test for it. > So it is not badly affecting the kernel checking for sure. That's indeed the case. But since there are regressions in the optimizations you can have at any time a regressions at the context checking, for example, as it's sometimes quite sensitive to it. > > 4) contains a beginner error > > Do you know if that follow up patch can fix it? I can't test that now. I'll do it later this evening (or tomorrow) even if I don't doubt that *this* *specific* issue is now fixed. > If that is the eventually right direction, it has bug > not as intended, then we fix the bug. > > If it has directional issue, then that is a different > story. Chris, I can't force you to take my word for it but I tell you now, once again and for the last time, that these two patches have others issues and that the pseudo-size solution is currently acceptable because of these issues. I'll be the first one ready to reevaluate it once the cast rework have been done. -- Luc "very very tired of this, lasting since March 2107" -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 25, 2018 at 11:56:17AM +0000, Dibyendu Majumdar wrote: > Hi Luc, > > On 25 February 2018 at 09:25, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > The right and wise thing would have to: > > 1) take the 2 patches that Jason is waiting for since 5 months > > (if you agree with). > > 2) take Randy's patch with the early return NULL > > 3) take Randy's patch to silence the indirect_branch attribute > > 4) take my patch to disable the default -Wunknown-attribute > > 5) inc the version number > > 6) push a release so that people stop seeing the current > > 148K+ warnings about the unknown indirect_branch attribute. > > > > But people are still waiting and seeing these 148K+ useless > > warnings and you have forced in a patch which: > > I am not sure why above is the 'right thing'. There does not appear to > be a definition or process to identify fixes or changes that people > need urgently - and if I recall correctly you previously objected to > having such a process when I suggested it. I don't remember we ever had a discussion about this. It's true though, that I don't believe in very strict procedures (though, I'm sure they are very good things in a lot of situations). I think we must adapt our decisions to the reality with all its complexity and unpredictability. > > 1) I explained to you why it was wrong > > Is it wrong because you disagree with it? I don't see a problem with > the approach. I tested your approach of OP_PUSH as well - and in my > view after comparing the two, this is more correct as it avoids > creating a pseudo IR instruction. It's true that the OP_PUSH solution adds more IR instructions. This doesn't make it incorrect. More importantly, it doesn't make the pseudo-size solution automatically correct. It's orthogonal to the correctness aspect. The technical aspects have been discussed endlessly between March and November, at which point I said that I was quite tired about it all and that I won't discuss about it anymore. During these months I explained several times: 1) that I tried the pseudo-size approach, even before Linus suggested the OP_ARG/OP_PUSH approach, and that I abandoned it because I saw several problems with it. 2) I explained (some of) the cause(s) of these problems 3) I said that I still think that *at term* the pseudo-size approach will probably be the right one but that *currently* it wasn't acceptable because of 2) 4) I also said that, even discarding the points here above, I was worried about how invasive Chris patch was and I warned him about its testing. In my tree, I didn't opted for the OP_PUSH solution but something totally different, maybe not ideal, admittingly a bit ugly but very direct, with very limited impact. Of course, the OP_PUSH solution and my current solution were both well tested and their impact well understood. This is definitively not something that can be said about what Chris put in the official tree. So, no, it's definitively not because 'I don't like' the pseudo-size solution and that I 'prefer' the OP_PUSH one. It's about making appropriate engineering choices. The elements are there, the code can be compared, the code can be tested. People can read the code and understand it, they can think by themselves and make their own choices. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 February 2018 at 17:49, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: >> I am not sure why above is the 'right thing'. There does not appear to >> be a definition or process to identify fixes or changes that people >> need urgently - and if I recall correctly you previously objected to >> having such a process when I suggested it. > > I don't remember we ever had a discussion about this. > It's true though, that I don't believe in very strict procedures > (though, I'm sure they are very good things in a lot of situations). > I think we must adapt our decisions to the reality with all its > complexity and unpredictability. > Here is the quote from previous discussion: >> 4. My suggestion would that for enhancements a simple majority voting >> should be adopted to ensure that features go in because Sparse users >> want them. > > I don't see a real need for that. > If someone has a features and wrote the patches, those patches should > be submitted, reviewed, ... as usual and then pushed to master. > I'm being a bit naive or optimistic here, but I think that for most > cases the usefullness of a features is obvious and would certainly not > need any kind of votes. If the features has some negatives aspects then > of course, it's the usual work to balance things and make the best choice. > > Also, for technical matters, I don't believe in votes. Only he technical > merrits should count. > > During these months I explained several times: > 1) that I tried the pseudo-size approach, even before Linus > suggested the OP_ARG/OP_PUSH approach, and that I abandoned > it because I saw several problems with it. From what I recall you tried adding a type to PSEUDO_VAL which didn't work. I don't think you tried the solution adopted by Chris. Here is your attempt: https://marc.info/?l=linux-sparse&m=148924738913671&w=2 In my view we should just go with Chris's approach and stop debating this issue endlessly. Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 25, 2018 at 06:38:45PM +0000, Dibyendu Majumdar wrote: > On 25 February 2018 at 17:49, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > >> I am not sure why above is the 'right thing'. There does not appear to > >> be a definition or process to identify fixes or changes that people > >> need urgently - and if I recall correctly you previously objected to > >> having such a process when I suggested it. > > > > I don't remember we ever had a discussion about this. > > It's true though, that I don't believe in very strict procedures > > (though, I'm sure they are very good things in a lot of situations). > > I think we must adapt our decisions to the reality with all its > > complexity and unpredictability. > > > > Here is the quote from previous discussion: > > >> 4. My suggestion would that for enhancements a simple majority voting > >> should be adopted to ensure that features go in because Sparse users > >> want them. Here above you was talking about 'fixes or changes people need urgently' while this suggestion was about simple enhancements. My objection was about using majority voting to take techncal decisions. > > During these months I explained several times: > > 1) that I tried the pseudo-size approach, even before Linus > > suggested the OP_ARG/OP_PUSH approach, and that I abandoned > > it because I saw several problems with it. > > From what I recall you tried adding a type to PSEUDO_VAL which didn't > work. I don't think you tried the solution adopted by Chris. > > Here is your attempt: > > https://marc.info/?l=linux-sparse&m=148924738913671&w=2 I had tried a pseudo-size approach before. I never posted it because of several issues I saw. The PSEUDO_VAL approach came later. It looked better at first but just after I posted it, I realized it couldn't work. Then Linus proposed the OP_ARG way which became OP_PUSH. As you know very well, it was easy, low impact, implemented and tested within hours by me and tested soon after by yourself. > In my view we should just go with Chris's approach and stop debating > this issue endlessly. The pseudo-size approach (the one I did or Chris' version, they're essentially the same patch) have issues with CSE and with casts, maybe others too. It's impact is not understood. It's not tested. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 February 2018 at 20:59, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Sun, Feb 25, 2018 at 06:38:45PM +0000, Dibyendu Majumdar wrote: >> On 25 February 2018 at 17:49, Luc Van Oostenryck >> <luc.vanoostenryck@gmail.com> wrote: >> >> I am not sure why above is the 'right thing'. There does not appear to >> >> be a definition or process to identify fixes or changes that people >> >> need urgently - and if I recall correctly you previously objected to >> >> having such a process when I suggested it. >> > >> > I don't remember we ever had a discussion about this. >> > It's true though, that I don't believe in very strict procedures >> > (though, I'm sure they are very good things in a lot of situations). >> > I think we must adapt our decisions to the reality with all its >> > complexity and unpredictability. >> > >> >> Here is the quote from previous discussion: >> >> >> 4. My suggestion would that for enhancements a simple majority voting >> >> should be adopted to ensure that features go in because Sparse users >> >> want them. > > Here above you was talking about 'fixes or changes people need urgently' > while this suggestion was about simple enhancements. > My objection was about using majority voting to take techncal decisions. > Voting can be used in two ways: a) to express need for a particular fix or feature b) to express approval for a change. Even technical merit is sometimes subjective - my solution is better than yours etc. - voting can help get opinions from others to decide the best way forward. >> > During these months I explained several times: >> > 1) that I tried the pseudo-size approach, even before Linus >> > suggested the OP_ARG/OP_PUSH approach, and that I abandoned >> > it because I saw several problems with it. >> >> From what I recall you tried adding a type to PSEUDO_VAL which didn't >> work. I don't think you tried the solution adopted by Chris. >> >> Here is your attempt: >> >> https://marc.info/?l=linux-sparse&m=148924738913671&w=2 > > I had tried a pseudo-size approach before. > I never posted it because of several issues I saw. > The PSEUDO_VAL approach came later. It looked better at first > but just after I posted it, I realized it couldn't work. > Then Linus proposed the OP_ARG way which became OP_PUSH. > As you know very well, it was easy, low impact, implemented and > tested within hours by me and tested soon after by yourself. > >> In my view we should just go with Chris's approach and stop debating >> this issue endlessly. > > The pseudo-size approach (the one I did or Chris' version, they're > essentially the same patch) have issues with CSE and with casts, > maybe others too. It's impact is not understood. It's not tested. > I think that is fine. We can test it and fix issues. I already tested it in my repo and did not find any issues - although I appear to have missed the bug you found. The way forward is to have it available and test it. If Chris can move the change to sparse-next branch then the objection to having this on HEAD / master is solved. Thanks and Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 24, 2018 at 03:30:44PM -0800, Christopher Li wrote: > On Sat, Feb 24, 2018 at 2:00 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > Hi Chris, > > > > I saw that a few patches have been pushed on the head, > > including the two I disagreed with and which I explained *why* > > they where wrong. Well. > > > > Meanwhile, I see some regressions with the current head. > > For example some simple programs like: > > void a(void) > > { > > int b[] = { 8 }; > > int c; > > for (;;) > > b[c] = b[0]; > > } > > now crashes when used with test-linearize. > > Oops, reproduced. Thanks for the report. > > I see pseudo->ident polluted by pseudo->size. > Separate them out of the union seems to fix the crash. This will increase the size of every pseudos. This will impact allocation time (which is not negligible for sparse), and cache hits. I'm sure you know that and have already checked its impact on speed since in the past it has been something so important to you. How much is it? 1%? Maybe 2%? More? -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 25, 2018 at 02:53:30PM -0800, Christopher Li wrote: > > https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=pseudo-size-fix The first tests pass without crashes now. I still see the regression(s). -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 25, 2018 at 4:24 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > This will increase the size of every pseudos. This will > impact allocation time (which is not negligible for sparse), > and cache hits. > > I'm sure you know that and have already checked its impact > on speed since in the past it has been something so important > to you. How much is it? 1%? Maybe 2%? More? The performance impact will be definitely less than 1% with that size increase one integer size. It should be almost not measurable. The value is only "long long". The size field can use a bit field without increase the structure size. I will send out that change later. You said you did not found crash in your first round testing. However there is some regression of the IR. Can you share the regression test case you found? Thanks Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 26, 2018 at 03:36:38PM -0800, Christopher Li wrote: > > You said you did not found crash in your first round testing. > However there is some regression of the IR. Can you share > the regression test case you found? I didn't spend much time at it but a typical small example of what I've seen is: void a(void) { long b; unsigned c = 0; for (;;) if (c) c = b; } Before it linearized as: .L0: br .L2 .L2: br .L2 Now it linearizes as: .L0: phisrc.32 %phi1(c) <- $0 br .L1 .L1: phi.32 %r1 <- %phi1(c), %phi2(c) cbr %r1, .L5, .L1 .L5: phisrc.32 %phi2(c) <- $0 br .L1 It's clearly one of the problem with casts and the CSE changes I've talked about. I'm sure that the fact that b is uninitialized has a role here in the ocurence of the problem but I'm convinced that it's independent of the problem itself. You can play with this example to create other cases with more dramatic differences, of course. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 26, 2018 at 5:12 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Mon, Feb 26, 2018 at 03:36:38PM -0800, Christopher Li wrote: >> >> You said you did not found crash in your first round testing. >> However there is some regression of the IR. Can you share >> the regression test case you found? > > I didn't spend much time at it but a typical small example > of what I've seen is: > void a(void) { > long b; > unsigned c = 0; > for (;;) > if (c) > c = b; > } > > Before it linearized as: > .L0: > br .L2 > .L2: > br .L2 > > Now it linearizes as: > .L0: > phisrc.32 %phi1(c) <- $0 > br .L1 > .L1: > phi.32 %r1 <- %phi1(c), %phi2(c) > cbr %r1, .L5, .L1 > .L5: > phisrc.32 %phi2(c) <- $0 > br .L1 > > It's clearly one of the problem with casts and the CSE changes > I've talked about. I'm sure that the fact that b is uninitialized First of all, that is a VALID IR of the original source code. I play with it a little more. void a(void) { long b = 1; int c = 0; for (;;) if (c) c = b; } } For both before and after the sized pseudo merge, it output: .L0: <entry-point> phisrc.32 %phi1(c) <- $0 br .L1 .L1: phi.32 %r1 <- %phi1(c), %phi2(c) cbr %r1, .L5, .L1 .L5: phisrc.32 %phi2(c) <- $1 br .L5 If we change to "long b=0;", both before and after the merge it output: .L0: <entry-point> br .L2 .L2: br .L2 So clearly sparse is not smart enough to see that C = 0 then the srcphi2(c) is never going to be executed. It is depending on phisrc2(c) and phisrc1(c) are the exact same pseudo (0) so it optimized away the %r1 phi instruction as 0. In the case b is not define then pick one value (1) then you get the long output and pick value (0) you get the short version. It is clear pointing towards b is not being initialized making the different here. How are you convinced it is independent of the problem itself? It is possible that CSE is seeing same value of constant has two different size might consider it as different pseudo. In those case we want to close examine how do it get into this situation in the first place. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27 February 2018 at 01:12, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Mon, Feb 26, 2018 at 03:36:38PM -0800, Christopher Li wrote: >> >> You said you did not found crash in your first round testing. >> However there is some regression of the IR. Can you share >> the regression test case you found? > > I didn't spend much time at it but a typical small example > of what I've seen is: > void a(void) { > long b; > unsigned c = 0; > for (;;) > if (c) > c = b; > } > Bear in mind that Sparse has always had issues with uninitialized vars as I reported a year ago. Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27 February 2018 at 03:30, Christopher Li <sparse@chrisli.org> wrote: > It is possible that CSE is seeing same value of constant has > two different size might consider it as different pseudo. In those > case we want to close examine how do it get into this situation > in the first place. > Hi Chris, does operand_size() in simplify need to look at pseudo value size? p.s. Apologies if you already received this message; I had two bounces from the mailing list because my mobile device included HTML content. -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27 February 2018 at 01:12, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Mon, Feb 26, 2018 at 03:36:38PM -0800, Christopher Li wrote: >> >> You said you did not found crash in your first round testing. >> However there is some regression of the IR. Can you share >> the regression test case you found? > > I didn't spend much time at it but a typical small example > of what I've seen is: > void a(void) { > long b; > unsigned c = 0; > for (;;) > if (c) > c = b; > } > Hi, I have checked above in my project - basically this failed before and it fails after the patch. As I mentioned, this issue has been discussed at length before - if a variable is uninitialized then it can cause problems in the simplification phase - especially if branching is involved. In my version _before_ I applied the patch: C:\work\github\dmr_c\tests\set1>..\..\build\Debug\linearize.exe -O1 crash2.c crash2.c:7:1: warning: no newline at end of file crash2.c:1:6: warning: symbol 'a' was not declared. Should it be static? a: .L0: <entry-point> store.32 $8 -> 0[b] phisrc.32 %phi1(b) <- $8 phisrc.32 %phi2 <- $8 phisrc.32 %phi3 <- $8 br .L1 .L1: load.32 %r1 <- 0[b] symaddr.64* %r2 <- b scast.64 %r4 <- (32) VOID muls.64 %r5 <- %r4, $4 add.64* %r6 <- %r2, %r5 store.32 %r1 -> 0[%r6] br .L1 It is the VOID that is the problem. Previously I reported: https://marc.info/?l=linux-sparse&m=149070715427276&w=2 https://marc.info/?l=linux-sparse&m=148983731629748&w=2 These reports led to the whole discussion on the problems with Sparse SSA implementation. I also do not think that one can have regression in Sparse IR as there is no defined correct IR for sparse. The correct IR is what gives the correct results - and that is why I use LLVM backend in all my tests as it tells me what the real outcome is. IR output changes from one change to another and just because something has changed it doesn't make it incorrect - the only measure is whether it caused existing tests to fail or resulted in wrong result from the compiled program. Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 27, 2018 at 5:30 AM, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote: > > > Hi Chris, does operand_size() in simplify need to look at pseudo value size? Not sure, operand_size() is only use in OP_ASR. As far as I can tell, sparse never generate OP_ASR, so that code path is not executed. The current operand_size() is returning the sign extended highest bit size. Sparse currently does sign extend before put into the value field. If you use the pseudo->size, it will behave differently than the current operand_size(). It does not do sign extended. My reading of simplify_asr(), that function seems a little bit problematic. ASR(-1, shift_bits) should always be -1, not 0. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 February 2018 at 10:01, Christopher Li <sparse@chrisli.org> wrote: > On Tue, Feb 27, 2018 at 5:30 AM, Dibyendu Majumdar > <mobile@majumdar.org.uk> wrote: >> >> >> Hi Chris, does operand_size() in simplify need to look at pseudo value size? > > Not sure, operand_size() is only use in OP_ASR. As far as I can tell, > sparse never generate OP_ASR, so that code path is not executed. > > The current operand_size() is returning the sign extended highest bit size. > Sparse currently does sign extend before put into the value field. > If you use the pseudo->size, it will behave differently than the current > operand_size(). It does not do sign extended. > > My reading of simplify_asr(), that function seems a little bit problematic. > ASR(-1, shift_bits) should always be -1, not 0. > Okay, so I am trying to understand where in the simplification phase the size of the value pseudo used. This was the only place I could find. Although now multiple value pseudos are created - I guess the actual value is still the same, and if the size of the value pseudo does not play a part in simplification then sure this change cannot cause a change in the simplification process? Or am I missing something? Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 28, 2018 at 10:59 AM, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote: > > Okay, so I am trying to understand where in the simplification phase > the size of the value pseudo used. This was the only place I could > find. Although now multiple value pseudos are created - I guess the > actual value is still the same, and if the size of the value pseudo > does not play a part in simplification then sure this change cannot > cause a change in the simplification process? Or am I missing > something? It is in theory possible in the following case, you have two instruction all the same except two operand size: %r10 <- op.32 %r11, $10 (size 8) %r13 <- op.32 %r11, $10 (size 32) In that case, CSE will not able to see the $10 as the same, because they have size difference and reference by two different pseudo value. If that ever happen, that is a strange situation to get into in the first place. We might want to look a closer look if that ever happen. How can same size instruction have different size operand. The fix I think just need to change the first $10(size 8) into the $10(size 32) version. It is a lot simpler than change the type. I am not sure this example is actually trigger able yet. Not big deal. The whole pesudo size change I feel is a lot simpler than the OP_PUSH or the call argument embed a type approach Luc was doing. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 February 2018 at 19:28, Christopher Li <sparse@chrisli.org> wrote: > On Wed, Feb 28, 2018 at 10:59 AM, Dibyendu Majumdar > <mobile@majumdar.org.uk> wrote: >> >> Okay, so I am trying to understand where in the simplification phase >> the size of the value pseudo used. This was the only place I could >> find. Although now multiple value pseudos are created - I guess the >> actual value is still the same, and if the size of the value pseudo >> does not play a part in simplification then sure this change cannot >> cause a change in the simplification process? Or am I missing >> something? > > It is in theory possible in the following case, you have two instruction all the > same except two operand size: > > %r10 <- op.32 %r11, $10 (size 8) > %r13 <- op.32 %r11, $10 (size 32) > > In that case, CSE will not able to see the $10 as the same, because they > have size difference and reference by two different pseudo value. > > If that ever happen, that is a strange situation to get into in the first place. > We might want to look a closer look if that ever happen. How can same size > instruction have different size operand. > > The fix I think just need to change the first $10(size 8) into the $10(size 32) > version. It is a lot simpler than change the type. I am not sure this example > is actually trigger able yet. Not big deal. > Okay thank you for the explanation - might be worth trying to create a test case that triggers this scenario. Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27 February 2018 at 01:12, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Mon, Feb 26, 2018 at 03:36:38PM -0800, Christopher Li wrote: >> >> You said you did not found crash in your first round testing. >> However there is some regression of the IR. Can you share >> the regression test case you found? > > I didn't spend much time at it but a typical small example > of what I've seen is: > void a(void) { > long b; > unsigned c = 0; > for (;;) > if (c) > c = b; > } > I am curious what is happening here. If I print off the value pseudos that are created then I get this: pseudo type = 3, size = 32, value = 0 pseudo type = 3, size = 32, value = 0 pseudo type = 3, size = 64, value = 0 Now where is the value pseudo of size 64 coming from as it is not in the code above? Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1 March 2018 at 20:57, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote: > On 27 February 2018 at 01:12, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: >> On Mon, Feb 26, 2018 at 03:36:38PM -0800, Christopher Li wrote: >>> >>> You said you did not found crash in your first round testing. >>> However there is some regression of the IR. Can you share >>> the regression test case you found? >> >> I didn't spend much time at it but a typical small example >> of what I've seen is: >> void a(void) { >> long b; >> unsigned c = 0; >> for (;;) >> if (c) >> c = b; >> } >> > > I am curious what is happening here. > If I print off the value pseudos that are created then I get this: > > pseudo type = 3, size = 32, value = 0 > pseudo type = 3, size = 32, value = 0 > pseudo type = 3, size = 64, value = 0 > > Now where is the value pseudo of size 64 coming from as it is not in > the code above? > Okay it seems to come from flow.c in find_dominating_stores() - in the call to convert_load_instruction(). Any explanation of what this is doing? Btw there are only two value pseudos created above - not three - that is just a typo. Thanks and Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/linearize.h b/linearize.h index fd8e00d3..7841ba23 100644 --- a/linearize.h +++ b/linearize.h @@ -31,11 +31,9 @@ enum pseudo_type { struct pseudo { int nr; enum pseudo_type type; + int size; /* OP_SETVAL only */ struct pseudo_user_list *users; - union { - struct ident *ident; - int size; /* OP_SETVAL only */ - }; + struct ident *ident; union { struct symbol *sym; struct instruction *def;