We don't want the existence of debug instructions affect codegen so we now
ignore debug instructions and other "isAssumeLikeIntrinsics in the
"extend schedule region" search loop in
BoUpSLP::BlockScheduling::extendSchedulingRegion.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
@jmorse : I saw you did a bunch of nice debug info cleanups in https://reviews.llvm.org/D151419 but I don't think this problem is fixed there?
I stumbled upon this problem in downstream testing where debug info made us hit the ScheduleRegionSize limit thing.
Comment Actions
What is the difference between runs results? Looks like it gets vectorized in all cases. Also, do a cleanup for metadata and attributes.
| llvm/test/Transforms/SLPVectorizer/X86/schedule_budget.ll | ||
|---|---|---|
| 8 ↗ | (On Diff #529575) | So this test case is kind of "broken" already on trunk? (Looks like commit 352c46e70716061e99cae2009daddbfc78380fda changed this test in a way so that the first loads are vectorized even with a budget at 16.) |
Comment Actions
Many thanks for the patch, it looks good to me although I'm not very familiar with the vectorisers -- if no-one else reviews deeply then I can give it a shot later,
Would it be simpler to replace the increment lines with calls to getNextNonDebugInstruction rather than add a new predicate / set of calls -- this has the benefit of being slightly smaller, and will make cleanup easier once we've suppressed debug intrinsics entirely,
@jmorse : I saw you did a bunch of nice debug info cleanups in https://reviews.llvm.org/D151419 but I don't think this problem is fixed there?
Those are just the least contentious changes, there's more at [0], and probably a few other patches too. Although it looks like I didn't detect this particular defect at all!
[0] https://github.com/jmorse/llvm-project/commit/2d3354323ae83df2b00dc327645435fae668f94c
| llvm/test/Transforms/SLPVectorizer/X86/schedule_budget.ll | ||
|---|---|---|
| 8 ↗ | (On Diff #529575) | Not broken, just changed. You'd better to add a new test with budget limits and debug info. |
| llvm/test/Transforms/SLPVectorizer/X86/schedule_budget.ll | ||
|---|---|---|
| 8 ↗ | (On Diff #529575) | Just thinking that this comment, as well as the later comment about " ; Don't vectorize these loads.", seem to explain the expected outcome of the test. But then the CHECK:s seem to validate something else. That is confusing. But maybe uabelho can just remove the RUN line with budget=16. One could just use budget=18 instead. As that also would show the debug-variance problem (e.g by pre-commit such a slightly modified test). |
| llvm/test/Transforms/SLPVectorizer/X86/schedule_budget.ll | ||
|---|---|---|
| 8 ↗ | (On Diff #529575) | The comments need to be removed in the separate patch. Or budget limit adjusted, whatever. |
Comment Actions
Created a new testcase instead of fiddling with the existing one.
Minor cleanup in metadata, removed dbg.value attributes.
Comment Actions
I don't know. The searches use both forward and reverse iterators, I thought changing to getNextNonDebugInstruction/getPrevNonDebugInstruction would make the change larger since I guess I'd have to get rid of the reverse iterator then (?) and since I'm not familiar with this code I tried keeping the change as small as possible to not risk messing things up.
But I can try to go that route again if we prefer that.
Comment Actions
But I can try to go that route again if we prefer that.
Not a strong opinion, it works just as well as it is.
| llvm/test/Transforms/SLPVectorizer/X86/schedule_budget_debug_info.ll | ||
|---|---|---|
| 179 | Try to simplify metadata, I rather doubt you need all the debug info specified here | |
Comment Actions
Rebase and remove as much metadata as I can without getting complaints from e.g. update_test_checks.py.
| llvm/test/Transforms/SLPVectorizer/X86/schedule_budget_debug_info.ll | ||
|---|---|---|
| 1 | Create a separate NFC patch with this test only. | |
Comment Actions
Precommit testcase in https://reviews.llvm.org/D152705 so this patch shows how the testcase is improved.
| llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
|---|---|---|
| 11460 | I think it worth it to extend it not only for debug intrinsics, but for all ephemeral instructions, like lifetime, assume, etc. | |
| llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
|---|---|---|
| 11460 | I can do that, e.g. to check isAssumeLikeIntrinsic(). However, then the patch is starting to drift away from just fixing the "dbg info affects codegen" problem I was aiming at so then we start fixing/changing something else as well. Do you still want that in the same patch? | |
| llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
|---|---|---|
| 11460 | Yes, the change is small, you can do it here. And rename the title. | |
I think it worth it to extend it not only for debug intrinsics, but for all ephemeral instructions, like lifetime, assume, etc.