Am Mi., 2. Okt. 2019 um 07:08 Uhr schrieb Florian Hahn via llvm-dev <llvm-dev at lists.llvm.org>:> The other thing is that with the patch behaviour is slightly changed and we could get a diagnostic we didn't get before: > > warning: loop not vectorized: the optimizer was unable to > perform the requested transformation; the transformation might be disabled or > specified as part of an unsupported transformation ordering > [-Wpass-failed=transform-warning] > > For the example given in revert 858a1ae, in both cases before and after my commit, the loop vectoriser was bailing because "Not vectorizing: The exiting block is not the loop latch".The source looks like a straightforward canonical loop. What pass transformed it to have code between the exiting block and the latch?> But the difference is that vectorize_width() now implies vectorize(enable), and so this is now marked as forced vectorisation which wasn't the case before. Because of this forced vectorization, and that the transformation wasn't applied, we now emit this diagnostic. The first part of this diagnostic is spot on: "the optimizer was unable to perform the requested transformation". We could argue about the suggestions given as to why the transformations didn't happen in this case, but overall I think this is an improvement.The patch that added the warning as-is was by me (https://reviews.llvm.org/D55288). It changed to emitted message from "loop not vectorized: failed explicitly specified loop vectorization" to the lengthier description following review feedback. It's done by the WarnMissedTransformation and just looks for transformation metadata that is still in the IR after all passes that should have transformed them have ran. That is, it does not know why it is still there -- it could be because the LoopVectorize pass is not even in the pipeline -- and we cannot be more specific in the message. However, -Rpass-missed=loop-vectorize may give more information.> The additional warning makes sense to me and I think is also beneficial to the user. > > Before, we silently ignored vector_width() in the example [1] and I suppose the user was expecting vectorize_width(4) to be honored. Now we are more transparent in informing the user what is happening: we were not able to honor their requested pragma and I assume they would be interested in knowing.As already mentioned, the loop indeed was never vectorized. It is again the question whether vectorize_width(4) means "vectorize with simd length 4" or "if this is vectorized, use simd length 4". Since the decided on the former (which is also what the docs say), the warning is correct. That is, IMHO, Chrome should either remove the #pragma (since it has no effect), add -Wno-pass-failed. We could also wait for the LoopVectorize pass to support this, Philip Reames is currently working on it. Michael
On 10/2/19 11:39 AM, Michael Kruse via llvm-dev wrote:> Am Mi., 2. Okt. 2019 um 07:08 Uhr schrieb Florian Hahn via llvm-dev > <llvm-dev at lists.llvm.org>: >> The other thing is that with the patch behaviour is slightly changed and we could get a diagnostic we didn't get before: >> >> warning: loop not vectorized: the optimizer was unable to >> perform the requested transformation; the transformation might be disabled or >> specified as part of an unsupported transformation ordering >> [-Wpass-failed=transform-warning] >> >> For the example given in revert 858a1ae, in both cases before and after my commit, the loop vectoriser was bailing because "Not vectorizing: The exiting block is not the loop latch". > The source looks like a straightforward canonical loop. What pass > transformed it to have code between the exiting block and the latch? > > >> But the difference is that vectorize_width() now implies vectorize(enable), and so this is now marked as forced vectorisation which wasn't the case before. Because of this forced vectorization, and that the transformation wasn't applied, we now emit this diagnostic. The first part of this diagnostic is spot on: "the optimizer was unable to perform the requested transformation". We could argue about the suggestions given as to why the transformations didn't happen in this case, but overall I think this is an improvement. > The patch that added the warning as-is was by me > (https://reviews.llvm.org/D55288). It changed to emitted message from > "loop not vectorized: failed explicitly specified loop vectorization" > to the lengthier description following review feedback. > > It's done by the WarnMissedTransformation and just looks for > transformation metadata that is still in the IR after all passes that > should have transformed them have ran. That is, it does not know why > it is still there -- it could be because the LoopVectorize pass is not > even in the pipeline -- and we cannot be more specific in the message. > However, -Rpass-missed=loop-vectorize may give more information.As I recall, there is some trade-off here because it's hard for a transformation to know that it's last - either the last run of that particular transformation in the pipeline or the last transformation in the pipeline that can service a particular transformation request (and this is especially true if there are multiple, separated pipelines involved, such as in LTO). This is why we did not have transformations warn if they can't perform the requested transformation for structural reasons - maybe they will be able to later. However, we also should improve the diagnostics in these cases. I recommend that we consider taking a kind of delayed-diagnostic approach. When a pass cannot perform the requested transformation, it records some rationale into the metadata. That rationale can be reported by WarnMissedTransformation, if available, to make the diagnostic more helpful. If the transformation is later actually performed, then the extra information is discarded along with the transformation metadata itself. -Hal> > >> The additional warning makes sense to me and I think is also beneficial to the user. >> >> Before, we silently ignored vector_width() in the example [1] and I suppose the user was expecting vectorize_width(4) to be honored. Now we are more transparent in informing the user what is happening: we were not able to honor their requested pragma and I assume they would be interested in knowing. > As already mentioned, the loop indeed was never vectorized. It is > again the question whether vectorize_width(4) means "vectorize with > simd length 4" or "if this is vectorized, use simd length 4". Since > the decided on the former (which is also what the docs say), the > warning is correct. > > That is, IMHO, Chrome should either remove the #pragma (since it has > no effect), add -Wno-pass-failed. We could also wait for the > LoopVectorize pass to support this, Philip Reames is currently working > on it. > > Michael > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev-- Hal Finkel Lead, Compiler Technology and Programming Languages Leadership Computing Facility Argonne National Laboratory
Am Mi., 2. Okt. 2019 um 15:56 Uhr schrieb Finkel, Hal J. <hfinkel at anl.gov>:> > It's done by the WarnMissedTransformation and just looks for > > transformation metadata that is still in the IR after all passes that > > should have transformed them have ran. That is, it does not know why > > it is still there -- it could be because the LoopVectorize pass is not > > even in the pipeline -- and we cannot be more specific in the message. > > However, -Rpass-missed=loop-vectorize may give more information. > > As I recall, there is some trade-off here because it's hard for a > transformation to know that it's last - either the last run of that > particular transformation in the pipeline or the last transformation in > the pipeline that can service a particular transformation request (and > this is especially true if there are multiple, separated pipelines > involved, such as in LTO). This is why we did not have transformations > warn if they can't perform the requested transformation for structural > reasons - maybe they will be able to later. However, we also should > improve the diagnostics in these cases.This was not the only consideration. With ordered transformations, such as vectorize after unroll-and-jam, the LoopVectorize does not even have a chance to analyze the code since it is located after the LoopUnrollAndJam pass. We would still warn that vectorization has not been performed.> I recommend that we consider taking a kind of delayed-diagnostic > approach. When a pass cannot perform the requested transformation, it > records some rationale into the metadata. That rationale can be reported > by WarnMissedTransformation, if available, to make the diagnostic more > helpful. If the transformation is later actually performed, then the > extra information is discarded along with the transformation metadata > itself.I like the idea, but I am not sure how helpful are messages such as "The exiting block is not the loop latch" or "Cannot identify array bounds" are to the end user. It would still be an improvement. If there is no diagnostic metadata, do we keep emitting the current message? If there already is an explanation metadata, does the new one override the old one or is it appended? We could also just a hint to the diagnostic such as "-Rpass=loop-vectorize may provide more information". Michael