Tweak ConstantHoisting on X86

[Michael Zolotukhin]

Hi,

I thought about adding one more test for this to make the motivation for this change clear.

Is it worth adding?



Thanks,
Michael

On Apr 30, 2014, at 10:22 PM, Michael Zolotukhin <mzolotukhin at apple.com> wrote:

> Thanks, Juergen!
> 
> Before check-in I’m going to run some additional testing and wait for an update from Quentin (and maybe comments from others).
> 
> Michael
> 
> On Apr 30, 2014, at 9:13 PM, Juergen Ributzka <juergen at apple.com> wrote:
> 
>> Thanks for fixing this Michael. LGTM
>> -Juergen
>> On Apr 30, 2014, at 7:49 AM, Michael Zolotukhin <mzolotukhin at apple.com> wrote:
>> 
>>> 
>>> On Apr 30, 2014, at 5:17 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>> 
>>>> ----- Original Message -----
>>>>> From: "Michael Zolotukhin" <mzolotukhin at apple.com>
>>>>> To: "llvm commits" <llvm-commits at cs.uiuc.edu>
>>>>> Sent: Wednesday, April 30, 2014 8:11:16 AM
>>>>> Subject: Tweak ConstantHoisting on X86
>>>>> 
>>>>> 
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> ConstantHoisting currently could lead to inefficient code on X86 (the
>>>>> test is attached). Recently Juergen has solved a similar issue on
>>>>> ARM64, so these changes do the same on X86. The change is to
>>>>> disallow hoisting of constants from shift instructions.
>>>>> 
>>>>> While the original issue that I’m fixing is on X86, the same problem
>>>>> could happen on other platforms. So, I prepared two versions of the
>>>>> patch: the first one touches only X86, and the other changes
>>>>> ConstantHoisting itself. Personally, I’d prefer the X86-only
>>>>> version, because I don’t know much about other targets affected. Any
>>>>> thoughts on this?
>>>> 
>>>> Yes, please use the x86-only version. I would not want exactly this behavior for PowerPC, for example.
>>> Sure.
>>> 
>>>> -Hal
>>>> 
>>>>> 
>>>>> There is one more important detail. With this change test
>>>>> CodeGen/X86/remat-invalid-liveness.ll stops to check what it’s
>>>>> supposed to check. Right now I just silenced it, but we probably
>>>>> want to rewrite and make it less fragile. Quentin, could you please
>>>>> take a look and help me with that?
>>> I’ve found a simple way to fix the test - as the current change prevents constants from hoisting, I did it in the test manually. Now it checks what it checked before. Also to make sure it’s still relevant I checked that it fails if r206060 is reverted.
>>> 
>>> Here is an updated patch:
>>> <ConstantHoisting-x86-v2.patch>
>>> 
>>> Is it ok to commit?
>>> 
>>> Thanks,
>>> Michael
>>> 
>>>>> 
>>>>> Thanks,
>>>>> Michael
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>> 
>>>> 
>>>> -- 
>>>> Hal Finkel
>>>> Assistant Computational Scientist
>>>> Leadership Computing Facility
>>>> Argonne National Laboratory
>>> 
>>> _______________________________________________
>>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140505/1266a97e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ConstantHoisting-x86-test.patch
Type: application/octet-stream
Size: 820 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140505/1266a97e/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140505/1266a97e/attachment-0001.html>