Regression with r200947: [CodeGenPrepare] Move away sign extensions that get in the way of addressing mode.

[Quentin Colombet]

On Feb 14, 2014, at 2:06 PM, Tom Stellard <tom at stellard.net> wrote:

> On Thu, Feb 13, 2014 at 09:39:52AM -0800, Quentin Colombet wrote:
>> Hi Tom,
>> On Feb 13, 2014, at 8:22 AM, Tom Stellard <tom at stellard.net> wrote:
>> 
>>> On Wed, Feb 12, 2014 at 02:55:57PM -0800, Quentin Colombet wrote:
>>>> 
>>>> On Feb 12, 2014, at 2:07 PM, Tom Stellard <tom at stellard.net> wrote:
>>>> 
>>>>> On Wed, Feb 12, 2014 at 01:43:25PM -0800, Quentin Colombet wrote:
>>>>>> Hi Tom,
>>>>>> 
>>>>>> I think you find a bug in TargetLoweringBase::isLegalAddressingMode.
>>>>>> 
>>>>>> Indeed, TargetLoweringBase::isLegalAddressingMode (which is what it is called in your case), says that 3*r + r is a valid addressing mode.
>>>>>> Basically, this returns true for every scale that is not 0, 1, and 2. I believe it misses a default case in the switch statement that return false.
>>>>>> 
>>>>>> Alternatively, you could have overloaded this function for your target, to be sure it accepts only what it should.
>>>>>> 
>>>>>> Assuming you fixes this problem, the promotion would still be performed because in that case the transformation is neutral (i.e., we do not create new instructions).
>>>>>> The difference is: instead of selecting:
>>>>>> %in + %sext
>>>>>> we would select:
>>>>>> %in + %promoted_mul
>>>>>> 
>>>>>> That said, the mul 64bit is not a legal operation on your platform (nor is mul i32), thus, I could drop the change if we did not fold the instruction into the addressing mode when the operation is not legal.
>>>>>> 
>>>>> 
>>>>> I think it makes sense to avoid the promotion if the result can't be
>>>>> folded into the addressing mode.
>>>> Well, that is a bit more complex than this.
>>>> When it is neutral, it may still be a good idea to perform the transformation because it may expose more simplifications (like sext + load).
>>>> However, I would prefer the promotion not to be smart about that, at least for now.
>>>> 
>>>> Therefore, I still think that checking for an  operation to be legal or not may still be a viable approach.
>>>> 
>>>> 
>>>>> How will you determine whether or not an
>>>>> operation is legal?
>>>> TLI.isOperationLegalOrCustom?
>>>> 
>>> 
>>> Ok, I didn't realize IR passes had access to this.
>>> 
>>>> See the attached patch for an example of implementation.
>>>> 
>>>> If this serves your purposes, let me know when you fixed the isLegalAddressingMode and I will commit this patch with your test case.
>>>> 
>>> 
>>> I just sent a patch to the list to fix isLegalAddressingMode.  I
>>> included the test case with that patch and marked it as XFAIL.  With
>>> this patch and your patch everything works fine.
>> Great!
>> I’ll wait for that patch to land before including this one.
>> 
> 
> I just committed this patch.  Make sure to remove XFAIL from the test case when you
> commit yours.  Thanks for fixing this.
Done in r201439.

Thanks,
-Quentin

> 
> -Tom
> 
>> Thanks,
>> -Quentin
>> 
>>> 
>>> Thanks,
>>> Tom
>>> 
>>>> As it is the matched thinks the mul is folded anyway, making the proposed tuning useless.
>>>> 
>>>> Thanks,
>>>> -Quentin
>>>> 
>>>>> 
>>>>> -Tom
>>>>> 
>>>>>> Thanks for the test case.
>>>>>> -Quentin
>>>>>> 
>>>>>> On Feb 12, 2014, at 12:58 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>>> 
>>>>>>> Sure, I’m looking into it.
>>>>>>> 
>>>>>>> -Quentin
>>>>>>> 
>>>>>>> On Feb 12, 2014, at 12:03 PM, Tom Stellard <tom at stellard.net> wrote:
>>>>>>> 
>>>>>>>> Hi Quentin,
>>>>>>>> 
>>>>>>>> I've discovered a regression with this commit, please see the attached test case.
>>>>>>>> In this case, CodeGenPrepare is promoting the mul in a sext + mul pattern even
>>>>>>>> though the addressing mode it is creating isn't legal.
>>>>>>>> 
>>>>>>>> One interesting thing about this test case is that if you remove the
>>>>>>>> nsw from the mul instruction, then the incorrect transform does not
>>>>>>>> take place.  I'm not sure why this matters.  Would you mind taking a look?
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Tom
>>>>>>>> <codegen-prepare-addrmode-sext.ll>
>>>>>>> 
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140214/4680daec/attachment.html>