[llvm-commits] [llvm] r61537 - in /llvm/trunk: lib/Transforms/Scalar/InstructionCombining.cpp test/Transforms/InstCombine/xor-demorgans.ll

[Bill Wendling]

On Jan 2, 2009, at 12:38 PM, Chris Lattner wrote:

> On Dec 31, 2008, at 5:18 PM, Bill Wendling wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=61537&view=rev
>> Log:
>> Add transformation:
>>
>> xor (or (icmp, icmp), true) -> and(icmp, icmp)
>>
>> This is possible because of De Morgan's law.
>
> Nice.  Note that this should only be done if the two compares each  
> only ->hasOneUse().  Otherwise, you're introducing new compares.
>
Okay.

> Also, there is already code that handles a top-level not, please  
> merge this into that code.  The code is:
>
>   // Is this a ~ operation?
>   if (Value *NotOp = dyn_castNotVal(&I)) {
>     // ~(~X & Y) --> (X | ~Y) - De Morgan's Law
>     // ~(~X | Y) === (X & ~Y) - De Morgan's Law
>
> I'm not sure why that code exists and then the "bool not" code is  
> separate, but it would be nice to merge them together. I realize  
> this existed before your change :)
>
I'll see what I can do.

>> +      if (Op0I) {
>> +        Value *A, *B;
>> +        if (match(Op0I, m_Or(m_Value(A), m_Value(B)))) {
>> +          ICmpInst *AOp = dyn_cast<ICmpInst>(A);
>> +          ICmpInst *BOp = dyn_cast<ICmpInst>(B);
>
> Can you use m_ICmp here?  Why not also handle fcmp?  fcmp is  
> invertible as well.
>
I suppose that it's okay. I was being conservative, because floating  
point compares can be tricky. :-)

> In practice, I think it would be better to simplify/merge this whole  
> class of transformations by adding a new pair of static function:
>
> bool isCheaplyInvertible(Value*);
> Value *getChealyInvertedValue(Value*);
>
> This would allow you to handle the icmp/fcmp/and "~" cases all  
> identically instead of having slightly different copies of the  
> various "demorgan" code.
>
Sure. That would be nice.

In actuality, the code I inserted seems to be pessimizing some of the  
performance tests. It's probably generating code that the back-end  
doesn't handle as well as it should. Further investigation is needed,  
and I'll probably end up #if-defing it out or removing it temporarily.  
When I figure out what the regression is, then I can reimplement it  
with the suggestions you mentioned here.

-bw