[llvm-commits] patch: folding (zext (and x, cst))

[Nick Lewycky]

On 15 June 2011 08:35, Eli Friedman <eli.friedman at gmail.com> wrote:

> On Wed, Jun 15, 2011 at 3:24 AM, Nick Lewycky <nicholas at mxc.ca> wrote:
> > Eli Friedman wrote:
> >>
> >> On Tue, Jun 7, 2011 at 2:46 AM, Nick Lewycky<nicholas at mxc.ca>  wrote:
> >>>
> >>> I have an unfinished patch. I was looking to optimize:
> >>>
> >>>  define i32 @test1(i8 %x) nounwind readnone {
> >>>    %A = and i8 %x, -32
> >>>    %B = zext i8 %A to i32
> >>>    ret i32 %B
> >>>  }
> >>>
> >>> which currently does a mov into %al, then the "and", then extends, into
> >>> doing a single extending mov, then an "and" in 32-bits. The rule I
> >>> decided
> >>> upon is "(zext (and x, cst)) ->  (and (anyext x), (zext cst))" in the
> DAG
> >>> combiner.
> >>
> >> Just a comment, without reading the patch: it would be much more
> >> conservative to fold (zext (and (load x), cst)) ->  (and (zextload x),
> >> (zext cst)).  The transform you're proposing is much less obviously
> >> beneficial.
> >
> > Sure, but the transform I proposed should turn into this one. Regardless,
> > since I wasn't able to resolve the collateral issues brought up by my
> first
> > patch, I've implemented the transform you described and its sext version.
> >
> > This is totally cargo-cult written, and I'm not sure what I'm doing
> wrong.
> > Do I need the safety check I have for !LN0->isVolatile()?
>
> No, don't think so; replacing a volatile load with a volatile load
> should be okay.
>

Ok, thanks.

> Should I have a
> > check for !LegalOperations?
>
> You need a check like "(!LegalOperations ||
> TLI.isOperationLegal(ISD::AND, VT))" to make sure you aren't creating
> an illegal AND.
>

Done.

> How about some calls to CombineTo?
>
> You need a CombineTo call to handle the chain on the load.
>

Okay, but CombineTo is undocumented. It appears to be CombineTo(Old, New) to
replace Old with New, so I tried calling it with CombineTo(LN0, ExtLoad) but
that triggered the assert on the first line of CombineTo. Could you explain
a little more about what I need to do?

> Patch attached, please review! Carefully! ;)
>
> Your use of ExtendUsesToFormExtLoad (and the !N0.hasOneUse() case in
> general) looks suspicious.
>

Cargo-culted from nearby. Look at the way "fold (zext (load x)) -> (zext
(truncate (zextload x)))" is handled. I think what's going on is that the
(load x) may have other uses, so the ExtendUsesToFormExtLoad function checks
whether inserting the necessary truncates would be free or the users could
be extended, etc.

I don't see any obvious issues besides the ones mentioned above.
>

Thanks!

Nick


>
> -Eli
>
> _______________________________________________
> 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/20110615/c3d11b01/attachment.html>