[llvm-commits] [llvm] r145563 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineCompares.cpp test/Transforms/InstCombine/compare-abs-nonzero.ll

[Duncan Sands]

Hi Peter,

> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Wed Nov 30 21:58:40 2011
> @@ -1795,6 +1795,20 @@
>     if (Value *V = SimplifyICmpInst(I.getPredicate(), Op0, Op1, TD))
>       return ReplaceInstUsesWith(I, V);
>
> +  // comparing -val or val with non-zero is the same as just comparing val
> +  // ie, (val != 0) == (-val != 0)

this comment made me think you were going to replace "(-val) != 0" with
"val != 0" as a general optimization.  It makes no mention of "abs" or select.
Can you please change it to explain what you are really up to.

> +  if (I.getPredicate() == ICmpInst::ICMP_NE&&  match(Op1, m_Zero()))
> +  {
> +    Value *Cond, *SubSrc, *SelectFalse;
> +    if (match(Op0, m_Select(m_Value(Cond), m_Sub(m_Zero(), m_Value(SubSrc)),

Instcombine has a special dyn_castNegVal for checking whether something is
"-val". Also, this logic seems too special: why should the "-val" be on the
false branch?  If it is the true branch your transform will fail even though
it would still be correct.

> +                            m_Value(SelectFalse)))) {
> +      if (SubSrc == SelectFalse) {
> +        return CmpInst::Create(Instruction::ICmp, I.getPredicate(),
> +                               SubSrc, Op1);
> +      }

Style: no curly brackets when there is only one statement.

> +    }
> +  }
> +

Ciao, Duncan.