On Friday 18 July 2008 00:36, Nick Lewycky wrote:> David Greene wrote: > > Is my analysis correct? If so, is the PHI code the culprit (for not > > returning the min of the KnownZero bits) or is the Shl code the culprit > > (for not paying attention to the Mask passed in (it right shifts it)? > > I think your analysis is correct, and that Shl -- and many of the other > operations (AShr, LShr, SExt, Add?, Call?) -- should be modified to > always respect Mask.After thinking about this some more, I'm not so sure. The Shl really DOES result in six low zero bits, so it should return that. That PHI node has semantics that it should take care of itself (taking the min of the known zero and one bits). It seems a little brittle to me to have the PHI pass its semantics down to children via the Mask. It should just do what it knows is right in the first place. That said, there are many places that don't respect the Mask. Closer reading of the comment leads me to believe the Mask is simply a time-saving device, not a correctness-enforcing mechanism. I've fixed the PHI analysis to do the min in our code and it fixes the testcase I was working on. Doing a min like this would also allow us to have PHI nodes compute known zero and one bits even when there isn't a recurrence. -Dave
On Jul 18, 2008, at 8:37 AM, David Greene wrote:> On Friday 18 July 2008 00:36, Nick Lewycky wrote: >> David Greene wrote: >>> Is my analysis correct? If so, is the PHI code the culprit (for not >>> returning the min of the KnownZero bits) or is the Shl code the >>> culprit >>> (for not paying attention to the Mask passed in (it right shifts >>> it)? >> >> I think your analysis is correct, and that Shl -- and many of the >> other >> operations (AShr, LShr, SExt, Add?, Call?) -- should be modified to >> always respect Mask. > > After thinking about this some more, I'm not so sure. The Shl > really DOES > result in six low zero bits, so it should return that. That PHI > node has > semantics that it should take care of itself (taking the min of the > known > zero and one bits). > > It seems a little brittle to me to have the PHI pass its semantics > down > to children via the Mask. It should just do what it knows is right > in the > first place. > > That said, there are many places that don't respect the Mask. Closer > reading of the comment leads me to believe the Mask is simply a > time-saving device, not a correctness-enforcing mechanism. > > I've fixed the PHI analysis to do the min in our code and it fixes the > testcase I was working on. Doing a min like this would also allow us > to have PHI nodes compute known zero and one bits even when there > isn't a recurrence.I believe your analysis of the problem here is correct. Thanks for finding this and figuring it out! If you can submit your fix, please do so, otherwise let me know. Dan
David Greene wrote:> On Friday 18 July 2008 00:36, Nick Lewycky wrote: >> David Greene wrote: >>> Is my analysis correct? If so, is the PHI code the culprit (for not >>> returning the min of the KnownZero bits) or is the Shl code the culprit >>> (for not paying attention to the Mask passed in (it right shifts it)? >> I think your analysis is correct, and that Shl -- and many of the other >> operations (AShr, LShr, SExt, Add?, Call?) -- should be modified to >> always respect Mask. > > After thinking about this some more, I'm not so sure. The Shl really DOES > result in six low zero bits, so it should return that. That PHI node has > semantics that it should take care of itself (taking the min of the known > zero and one bits). > > It seems a little brittle to me to have the PHI pass its semantics down > to children via the Mask. It should just do what it knows is right in the > first place. > > That said, there are many places that don't respect the Mask. Closer > reading of the comment leads me to believe the Mask is simply a > time-saving device, not a correctness-enforcing mechanism.That's fine, but if you fix it that way, please audit InstructionCombiner SimplifyDemandedBits, which I believe has the same bug.> I've fixed the PHI analysis to do the min in our code and it fixes the > testcase I was working on. Doing a min like this would also allow us > to have PHI nodes compute known zero and one bits even when there > isn't a recurrence.Great! Did you commit a patch for this? Nick
On Saturday 19 July 2008 23:53, Nick Lewycky wrote:> > That said, there are many places that don't respect the Mask. Closer > > reading of the comment leads me to believe the Mask is simply a > > time-saving device, not a correctness-enforcing mechanism. > > That's fine, but if you fix it that way, please audit > InstructionCombiner SimplifyDemandedBits, which I believe has the same bug.Ok.> > I've fixed the PHI analysis to do the min in our code and it fixes the > > testcase I was working on. Doing a min like this would also allow us > > to have PHI nodes compute known zero and one bits even when there > > isn't a recurrence. > > Great! Did you commit a patch for this?Not yet. :( I am waiting for some paperwork on this end. Hopefully we only have to go through this pain once and then I can be much more active. I've been told it should be approved by the end of the month. -Dave