[llvm] r214011 - [x86] Fix PR20355 (for real). There are many layers to this bug.

[Quentin Colombet]

Hi Chandler, hi Ben,

I haven’t been able to come up with a test case that uses the low part of MUL_LOHI, even with Ben’s suggestion.

The questions now are:
1. Do we leave the code as is and wait for it to fail?
or
2. Do we fix the mask as I think Ben, You, and I agree that this is wrong?

I would prefer #2, though #1 would give us a test case when it eventually fails!

What do you think?

Thanks,
-Quentin

On Aug 5, 2014, at 10:28 AM, Quentin Colombet <qcolombet at apple.com> wrote:

> Thanks Ben!
> 
> I’ll see if I can expose the bug with that approach.
> 
> -Quentin
> 
>> On Aug 5, 2014, at 2:02 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>> 
>> On Mon, Aug 4, 2014 at 7:48 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>> Ping?
>>> 
>>> Benjamin, any idea on how we can write a test for the lowering of the low
>>> part of the MUL_LOHI on x86?
>> 
>> The only way I can think of is having a wide mul in IR and let the
>> legalizer emit a MUL_LOHI. I don't know what magic incantation is
>> needed for that though :/
>> 
>> - Ben
>> 
>>> 
>>> Thanks,
>>> -Quentin
>>> 
>>> On Jul 29, 2014, at 9:22 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>> 
>>> 
>>> On Jul 28, 2014, at 2:21 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>> 
>>> 
>>> On Jul 28, 2014, at 12:58 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
>>> 
>>> 
>>> On Mon, Jul 28, 2014 at 12:46 PM, Quentin Colombet <qcolombet at apple.com>
>>> wrote:
>>>> 
>>>> I think we still have a bug in the current implementation.
>>>> See my inline comments.
>>> 
>>> 
>>> Seems likely. Can you craft a test case to demonstrate it?
>>> 
>>> 
>>> I will see if I can come up with something.
>>> 
>>> 
>>> I haven’t found anything yet. The case I know that was using this path (div
>>> -> mul) just uses the high part of the result. Thus, the problem does not
>>> show up.
>>> 
>>> CC’ed Benjamin, he may have an idea on how to test that.
>>> 
>>> Cheers,
>>> -Quentin
>>> 
>>> 
>>> 
>>>> 
>>>> 
>>>> Also, I think it would be nice to update the comments on the masks value
>>>> instead of removing them.
>>>> When I reversed engineered this code, it took me ages to figure out what
>>>> were these magic values.
>>> 
>>> 
>>> I'm not sure that's going to help. The problem with comments is that they
>>> can get out of sync. I also don't think that little endian offsets are
>>> surprising or need lots of clarification here? The 1 instead of 0 thing you
>>> point out screams wrong to me at a glance, and I don't think a comment would
>>> really make it better (or worse, unless it got out of date).
>>> 
>>> 
>>> Fair enough!
>>> 
>>> -Quentin
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>>> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits