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

[Eli Friedman]

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.

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

> How about some calls to CombineTo?

You need a CombineTo call to handle the chain on the load.

> Patch attached, please review! Carefully! ;)

Your use of ExtendUsesToFormExtLoad (and the !N0.hasOneUse() case in
general) looks suspicious.

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

-Eli