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

[Eli Friedman]

On Wed, Jun 15, 2011 at 2:26 PM, Nick Lewycky <nlewycky at google.com> wrote:
> On 15 June 2011 10:37, Eli Friedman <eli.friedman at gmail.com> wrote:
>>
>> On Wed, Jun 15, 2011 at 10:19 AM, Nick Lewycky <nlewycky at google.com>
>> wrote:
>> >> > 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?
>>
>> Err, try ReplaceAllUsesOfValueWith.
>
> It's almost ReplaceLoadWithPromotedLoad, but I need to use the Trunc value
> for more things. I ended up not doing this and going back to CombineTo.
>>
>> >> > 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.
>>
>> Your current patch ends up duplicating the load; if you really want to
>> handle the case where the loaded value has multiple uses, look at what
>> the existing uses of ExtendUsesToFormExtLoad do.
>
> Okay, I pulled this thread and ended up refactoring. I've only tested the
> resulting patch on make check + the llvm nightly test suite, but I'm much
> closer to confident in its correctness. Updated patch attached.
> Thanks for the review so far!

I don't see any issues.  It would be nice to have a more thorough test
case which covers the cases where the load has multiple uses, though.

-Eli