[PATCH] D53356: [InstCombine] Teach the move free before null test opti how to deal with noop casts

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Sanjay,

> This is adding to a transform that seems out-of-place for instcombine:
>  It's a hoist transform that relies on SimplifyCFG to do the real optimization. Does the whole thing belong there (or CGP) instead?

Interestingly I think we had the exact same conversation when it was first added back in 2013, but I cannot find any trace of this discussion... My mind must play me tricks :).
I am happy to move the whole thing in simplifyCFG. That said, strictly speaking that combine doesn't modify the CFG.
CGP seems too late to me.

> It's the only transform in instcombine that is gated by 'minsize', so it's a limited optimization rather than a canonicalization?

That is correct, though, I'd argue that a lot of code in instcombine are optimizations too (as opposed to pure cannonicalization.)

How would you like to proceed?

Cheers,
-Quentin



================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2363-2367
+  if (!match(TI,
+             m_Br(m_ICmp(Pred, m_Specific(Op), m_Zero()), TrueBB, FalseBB)) &&
+      !match(TI,
+             m_Br(m_ICmp(Pred, m_Specific(Op->stripPointerCasts()), m_Zero()),
+                  TrueBB, FalseBB)))
----------------
lebedev.ri wrote:
> ```
> if (!match(TI, m_Br(m_ICmp(Pred,
>                            m_CombineOr(m_Specific(Op),
>                                        m_Specific(Op->stripPointerCasts())),
>                            m_Zero()), TrueBB, FalseBB)))
> 
> ```
Ah great! I was looking for something like m_CombineOr, but missed it.


Repository:
  rL LLVM

https://reviews.llvm.org/D53356