Page MenuHomePhabricator

InstCombine: Don't rewrite phi-of-bitcast when the phi has other users
ClosedPublic

Authored by cwabbott on Dec 9 2019, 7:12 AM.

Details

Summary

Judging by the existing comments, this was the intention, but the
transform never actually checked if the existing phi's would be removed.
See https://bugs.llvm.org/show_bug.cgi?id=44242 for an example where
this causes much worse code generation on AMDGPU.

Diff Detail

Event Timeline

cwabbott created this revision.Dec 9 2019, 7:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 7:12 AM
spatel added a subscriber: spatel.

I'm not sure if these patches conflict, but D71164 is also trying to fix a bug in this code.

I agree that we shouldn't be increasing instruction count, especially PHI count.

llvm/test/Transforms/InstCombine/pr44242.ll
1

Please autogenerate and precommit tests.

cwabbott marked an inline comment as done.Dec 9 2019, 7:42 AM
cwabbott added inline comments.
llvm/test/Transforms/InstCombine/pr44242.ll
1

Sorry, but what exactly do you mean?

cwabbott marked an inline comment as not done.Dec 9 2019, 7:51 AM
nikic added a comment.Dec 9 2019, 10:10 AM

Change looks fine to me. I'd suggest to add test coverage for a few more of the cases you check. In particular, right now you only cover the case where the outer phi has extra uses, but not the case where an inner phi has them.

llvm/test/Transforms/InstCombine/pr44242.ll
1

You can use utils/update_test_checks.py to generate the expected output. Then you should first commit just the test (with generated output without your patch). The patch will then only show the IR diff in the test, making is clearer what actually changed.

lebedev.ri added inline comments.Dec 9 2019, 12:02 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2263

I think we should be doing those checks here.

nikic added inline comments.Dec 9 2019, 12:07 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2263

The OldPhiNodes.count(PHI) == 0 part of the check wouldn't work correctly at this points -- it needs all PHIs to be collected first.

lebedev.ri added inline comments.Dec 9 2019, 12:24 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2263

Right. But the rest can be done here, no?

nikic added inline comments.Dec 9 2019, 12:29 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2263

While it could be done, I don't think there'd be much benefit. We'd have to iterate all the users a second time just to check the phi nodes. The current implementation is also nicely symmetric between the checking loop (added in this patch) and the replacement loop (preexisting), and I think it's worth keeping that symmetry.

lebedev.ri added inline comments.Dec 9 2019, 12:54 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2263

I don't have a strong opinion *here*, but in general the earlier we error-out
the less pointless work that was in vain (read: compile-time cost) we waste.

cwabbott added inline comments.Dec 10 2019, 3:16 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2263

I'd agree with @nikic here that this sounds like a premature optimization. It would split up the two parts which must be in sync into three, and break the symmetry between them, making it even harder to verify that the already-complex transform is correct, all for a theoretical performance benefit. I can do it if you require it, but otherwise I'd rather not.

foad added a subscriber: foad.Dec 10 2019, 3:31 AM
foad added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2360–2370

In a Release build I get "warning: unused variable" for TyA and TyB.

cwabbott updated this revision to Diff 233060.Dec 10 2019, 4:55 AM
  • Update precommitted test in this commit.
  • Make the test match the original bug better. It turns out that in my attempt to make it harder for other transforms to happen beforehand and break the test, I accidentally made another transform kick in which broke it anyways. Use this form with a loop which should hopefully defeat other optimizations better.
cwabbott updated this revision to Diff 233066.Dec 10 2019, 5:25 AM
cwabbott marked an inline comment as done.

Add diff for new test.

cwabbott updated this revision to Diff 233068.Dec 10 2019, 5:38 AM

Suppress unused variable warnings in release builds.

cwabbott marked an inline comment as done.Dec 10 2019, 5:39 AM
cwabbott added inline comments.
llvm/test/Transforms/InstCombine/pr44242.ll
1

Hopefully I did that right.

cwabbott marked an inline comment as not done.Dec 10 2019, 5:40 AM
nikic accepted this revision.Dec 10 2019, 5:51 AM

LGTM, thanks for adding the extra test coverage.

This revision is now accepted and ready to land.Dec 10 2019, 5:51 AM

Do you plan to land this soon?

I just asked earlier this week to get my commit rights back, but I haven't heard back right away, so you can commit it if I don't get it first.

mareko added a subscriber: mareko.Dec 30 2019, 9:44 AM

Would somebody please push this?

This revision was automatically updated to reflect the committed changes.