[PATCH] D85159: [ConstProp] Remove ConstantPropagation

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/test/Analysis/ConstantFolding/vscale.ll:210
-;
-  %i1 = insertelement <vscale x 4 x i32> undef, i32 1, i32 0
-  %i2 = shufflevector <vscale x 4 x i32> %i1, <vscale x 4 x i32> undef, <vscale x 4 x i32> zeroinitializer
----------------
aeubanks wrote:
> nikic wrote:
> > aeubanks wrote:
> > > fhahn wrote:
> > > > why are those removed? Is that functionality missing from instsimplify?
> > > Looks like in ConstProp it always calls `ConstantFoldInstruction()`, which in this case just returns the same value, and replaces all uses, inlining into the ret. But InstSimplify calls `SimplifyInstruction()` which special cases vector instructions but doesn't know how to simplify these values any more, so it returns nullptr and InstSimplify doesn't do anything.
> > > 
> > > Maybe inlining these values into the ret isn't super useful?
> > Sounds like an InstSimplify bug. SimplifyInstruction is supposed to be a superset of ConstantFoldInstruction.
> I took a look at this. It seems like insertelement constants aren't handled as well as insertelement instructions.
> 
> For example, at [1], an instruction is properly handled, causing 
> ```
> define i32 @foo() {
>   %v = insertelement <vscale x 4 x i32> undef, i32 1, i64 4
>   %r = extractelement <vscale x 4 x i32> %v, i64 4
>   ret i32 %r
> }
> ```
> to become
> ```
> define i32 @foo() {
>   ret i32 1
> }
> ```
> But if we start inlining vector constants then it ends up at
> ```
> define i32 @foo() {
>   ret i32 extractelement (<vscale x 4 x i32> insertelement (<vscale x 4 x i32> undef, i32 1, i64 4), i64 4)
> }
> ```
> since `llvm::findScalarElement` doesn't properly handle insertelement constants.
> 
> This can of course be fixed (I'm struggling to figure out how to peek into an `InsertElementConstantExpr` though...), but I wonder about the extra work of handling both insertelement instructions and constants everywhere and how maintainable that is.
> 
> Thoughts?
> 
> [1]: https://github.com/llvm/llvm-project/blob/fb04d7b4a69831f6b999b1776da738557b108e0d/llvm/lib/Analysis/VectorUtils.cpp#L282
Actually now these are fixed, see the description for what test changes have been made, should be better now.


================
Comment at: llvm/test/Transforms/Inline/externally_available.ll:1
-; RUN: opt < %s -inline -constprop -S | FileCheck %s
 
----------------
nikic wrote:
> I'd drop the `-constprop` from this test entirely and regenerate check lines. It doesn't really look relevant.
Done in https://reviews.llvm.org/D86653


================
Comment at: llvm/test/Transforms/Reassociate/2002-05-15-SubReassociate.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt < %s -reassociate -constprop -instcombine -dce -S | FileCheck %s
+; RUN: opt < %s -reassociate -instsimplify -instcombine -dce -S | FileCheck %s
 
----------------
nikic wrote:
> The `-instsimplify` here can probably just dropped (instcombine already does instsimplify internally). Same for the other llvm/test/Transforms/Reassociate tests.
Done in https://reviews.llvm.org/D86653


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85159/new/

https://reviews.llvm.org/D85159