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

[Quentin Colombet]

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.

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/20140213/6ee3ea45/attachment.html>