Tweak ConstantHoisting on X86

[Hal Finkel]

----- Original Message -----
> From: "Michael Zolotukhin" <mzolotukhin at apple.com>
> To: "Juergen Ributzka" <juergen at apple.com>
> Cc: "LLVM Commits" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, May 5, 2014 8:09:22 AM
> Subject: Re: Tweak ConstantHoisting on X86
> 
> 
> Hi,
> 
> 
> I thought about adding one more test for this to make the motivation
> for this change clear.
> 
> 
> Is it worth adding?

I think tests are almost always a good idea, this looks good to me.

 -Hal

> 
> 
> 
> 
> 
> 
> 
> 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
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory