Page MenuHomePhabricator

[ConstProp] Remove ConstantPropagation
ClosedPublic

Authored by aeubanks on Aug 3 2020, 2:11 PM.

Details

Summary

As discussed in
http://lists.llvm.org/pipermail/llvm-dev/2020-July/143801.html.

Currently no users outside of unit tests.

Replace all instances in tests of -constprop with -instsimplify.
Notable changes in tests:

  • vscale.ll - @llvm.sadd.sat.nxv16i8 is evaluated by instsimplify, use a fake intrinsic instead
  • InsertElement.ll - insertelement undef is removed by instsimplify in @insertelement_undef

llvm/test/Transforms/ConstProp moved to llvm/test/Transforms/InstSimplify/ConstProp

Diff Detail

Event Timeline

aeubanks created this revision.Aug 3 2020, 2:11 PM
Herald added a project: Restricted Project. · View Herald Transcript
aeubanks requested review of this revision.Aug 3 2020, 2:11 PM
fhahn added inline comments.Aug 7 2020, 1:32 PM
llvm/test/Analysis/ConstantFolding/vscale.ll
210

why are those removed? Is that functionality missing from instsimplify?

llvm/test/Transforms/ConstProp/bitcast.ll
2

I think we should probably move the tests to the instsimplify dir?

lattner accepted this revision.Aug 9 2020, 11:10 PM

nice, thanks for cleaning this up!

This revision is now accepted and ready to land.Aug 9 2020, 11:10 PM
aeubanks updated this revision to Diff 284439.Aug 10 2020, 10:43 AM

Move tests into InstSimplify/ConstProp

aeubanks marked an inline comment as done.Aug 10 2020, 10:43 AM
aeubanks added inline comments.
llvm/test/Analysis/ConstantFolding/vscale.ll
210

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?

nikic added a subscriber: nikic.Aug 10 2020, 12:18 PM
nikic added inline comments.
llvm/test/Analysis/ConstantFolding/vscale.ll
210

Sounds like an InstSimplify bug. SimplifyInstruction is supposed to be a superset of ConstantFoldInstruction.

aeubanks added inline comments.Aug 10 2020, 4:49 PM
llvm/test/Analysis/ConstantFolding/vscale.ll
210

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

aeubanks updated this revision to Diff 288074.Aug 26 2020, 12:20 PM

Rebase after instsimplify fixes

aeubanks edited the summary of this revision. (Show Details)Aug 26 2020, 12:21 PM
aeubanks edited the summary of this revision. (Show Details)

Can you please also move Analysis/ConstantFolding/* and Other/2002-03-11-ConstPropCrash.ll into Transforms/InstSimplify?

llvm/test/Transforms/Inline/externally_available.ll
1 ↗(On Diff #288074)

I'd drop the -constprop from this test entirely and regenerate check lines. It doesn't really look relevant.

llvm/test/Transforms/Reassociate/2002-05-15-SubReassociate.ll
2 ↗(On Diff #288074)

The -instsimplify here can probably just dropped (instcombine already does instsimplify internally). Same for the other llvm/test/Transforms/Reassociate tests.

aeubanks updated this revision to Diff 288095.Aug 26 2020, 1:18 PM

Move more tests into Transforms/InstSimplify/ConstProp
Rebase past https://reviews.llvm.org/D86653

aeubanks added inline comments.Aug 26 2020, 1:20 PM
llvm/test/Analysis/ConstantFolding/vscale.ll
210

Actually now these are fixed, see the description for what test changes have been made, should be better now.

llvm/test/Transforms/Inline/externally_available.ll
1 ↗(On Diff #288074)
llvm/test/Transforms/Reassociate/2002-05-15-SubReassociate.ll
2 ↗(On Diff #288074)
nikic accepted this revision.Aug 26 2020, 1:20 PM

LGTM

This revision was automatically updated to reflect the committed changes.

@aeubanks This change seems to have broken an OCaml test. Could you please take a look and push a fix at your earliest convenience?

Before: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/35253

After: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/35254

Sorry, I thought I fixed that shortly after in 82875dcf, but apparently I
missed some more, hopefully fixed in dd04fa17. Let me know if there are
still issues.

mesa/llvmpipe was using this pass, so it doesn't build anymore. If the proposed replacement is the InstSimplifyLegacyPass that's fine, however the old one was nicely available via the c interface (LLVMAddConstantPropagationPass) whereas the InstSimplifyLegacyPass is not.
(I'd note that we only use very few passes, and they are perhaps not optimal, but the goal is to avoid any expensive passes while still getting some optimization. So, not really insisting on InstSimplifyLegacyPass neither if there are other suitable replacements doing roughly the same thing - it would be nice though if we'd not have to do our own c++ wrapper as we try to avoid that when possible.)

Yeah InstSimplify really should be used anywhere ConstProp is being used,
adding C bindings for it in https://reviews.llvm.org/D86764.