Tweak ConstantHoisting on X86

[Eric Christopher]

The additional test is very nice, thank you.

-eric

On Mon, May 5, 2014 at 6:09 AM, Michael Zolotukhin
<mzolotukhin at apple.com> wrote:
> 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>