[PATCH] D49151: [SimplifyIndVar] Avoid generating truncate instructions with non-hoisted Load operand.

[Abderrazek Zaafrani via Phabricator via llvm-commits]

az added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:1460
+  auto *WideBO = BinaryOperator::Create(NarrowBO->getOpcode(), LHS, RHS,
+                                        NarrowBO->getName());
+  IRBuilder<> Builder(NarrowUse);
----------------
efriedma wrote:
> az wrote:
> > efriedma wrote:
> > > It probably isn't a good idea to create a new multiply without erasing the old one.
> > > 
> > > 
> > Actually, this is cloning the use instruction and not removing it yet. The old instruction is removed by the defining instruction when we return up the call chain if it has no other use. This is my understanding of how things worked with widening the use. However, I am adding some code in the new revision to immediately remove the new instruction when not useful. I used to leave the instruction unused and hoping that it will be removed by dead code elimination.  
> We need to make sure to avoid the situation where after this transform runs, there's an i32 multiply and an i64 multiply.  It's possible IV rewriting could lead to this sort of situation anyway in certain edge cases, I guess (haven't looked too closely), but multiplies are relatively expensive, and i64 multiplies are more expensive than i32 multiplies.
> 
> Actually, more generally, we probably need to weigh the extra cost of the multiply; on multiple targets, an i64 multiply is substantially slower than an i32 multiply.
Good catch.
I updated the patch so that it does not generate two versions of the instructions, an i32 and i64 (for the example below a two versions of the multiply is a possibility). I have done so by making sure that the instruction in question is consumed by a s/z extend instruction to get a full benefit of widening and eliminate the extend instruction. Having said that, it is most likely the original code is suffering from the same issue of generating multiple version of the instructions. I will look at it post the patch. 
As for the general idea of adding a cost model for the widening optimization, it is worth investigating it (may be along with the other optimizations within SimplifyIndVar in case they do not have a cost model). In case you are aware of such examples, please share but I think you have good point. We are transforming the code by widening and elimination of some instructions without checking on profitability.  


https://reviews.llvm.org/D49151