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

[Quentin Colombet]

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?

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.

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/20140212/3d7d809a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sext-tuning.patch
Type: application/octet-stream
Size: 2880 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140212/3d7d809a/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140212/3d7d809a/attachment-0001.html>