This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Fixing build warning with llvm-mca
ClosedPublic

Authored by plotfi on Jan 24 2021, 4:11 PM.

Details

Summary

Warning is generated because the range based for-loop is always copying by value but uses a ref &:

[4083/4581] Building CXX object tools/llvm-mca/CMakeFiles/llvm-mca.dir/Views/InstructionInfoView.cpp.o
/path/to/Projects/llvm-project/llvm/tools/llvm-mca/Views/InstructionInfoView.cpp:96:20: warning: loop variable 'I' is always a copy because the range of type 'detail::zippy<detail::zip_shortest, ArrayRef<MCInst>, MutableArrayRef<InstructionInfoViewData> &>' does not return a reference [-Wrange-loop-analysis]
  for (const auto &I : zip(getSource(), IIVD)) {
                   ^
/path/to/Projects/llvm-project/llvm/tools/llvm-mca/Views/InstructionInfoView.cpp:96:8: note: use non-reference type 'std::__1::tuple<const llvm::MCInst &, llvm::mca::InstructionInfoView::InstructionInfoViewData &>'
  for (const auto &I : zip(getSource(), IIVD)) {
       ^~~~~~~~~~~~~~~
1 warning generated.

Diff Detail

Event Timeline

plotfi created this revision.Jan 24 2021, 4:11 PM
plotfi requested review of this revision.Jan 24 2021, 4:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2021, 4:11 PM
andreadb accepted this revision.Jan 25 2021, 3:45 AM
andreadb added a subscriber: lebedev.ri.

LGTM.

For the record, that warning was firstly introduced by another commit which was meant to be an NFC refactoring
in preparation for the most recent work that added support for the generation of JSON files.

commit cf6adecd6a8718ee2737ca55e4cd938364b984cc
[llvm-mca][NFC] Refactor views to separate data collection from printing.

That change was reviewed by @lebedev.ri.
Using zip may be sub-optimal because it introduces copies of every MCInst in source.
While it is true that our typical source size is often less than 16, there may be some
rare cases where the source is much larger.
I am not a big fan of using zip in this particular case. However the cost of using zip is probably negligible.

This revision is now accepted and ready to land.Jan 25 2021, 3:45 AM

LGTM.

For the record, that warning was firstly introduced by another commit which was meant to be an NFC refactoring
in preparation for the most recent work that added support for the generation of JSON files.

commit cf6adecd6a8718ee2737ca55e4cd938364b984cc
[llvm-mca][NFC] Refactor views to separate data collection from printing.

That change was reviewed by @lebedev.ri.

Using zip may be sub-optimal because it introduces copies of every MCInst in source.

Citation needed.
Even the warning says otherwise.

While it is true that our typical source size is often less than 16, there may be some
rare cases where the source is much larger.
I am not a big fan of using zip in this particular case. However the cost of using zip is probably negligible.

BTW, https://reviews.llvm.org/D86644?id=318269 likely added a few more of these bugs.

BTW, https://reviews.llvm.org/D86644?id=318269 likely added a few more of these bugs.

Oh I see. I completely missed those changes from auto I to const auto &I.
That's why I thought it was from a previous commit.
Apologies for that.

LGTM.

For the record, that warning was firstly introduced by another commit which was meant to be an NFC refactoring
in preparation for the most recent work that added support for the generation of JSON files.

commit cf6adecd6a8718ee2737ca55e4cd938364b984cc

Citation needed.
Even the warning says otherwise.

Right. I completely misunderstood that warning.
I wrongly thought that somehow llvm::zip was introducing copies under the hood. But as you wrote, that is not what the warning is saying and zip doesn't work that way.
Sorry about the confusion. You are absolutely right and the patch still looks good to me.

lebedev.ri accepted this revision.Jan 25 2021, 4:45 AM

Ping.

@plotfi could you please commit this?

Thanks

RKSimon closed this revision.Feb 4 2021, 9:52 AM
RKSimon added a subscriber: RKSimon.