This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Adjust SmallVector.h header to workaround XL build compiler issue
ClosedPublic

Authored by Xiangling_L on Mar 12 2021, 1:37 PM.

Details

Summary

Previously delivered a patch to workaround the building issue related to SmallVector are here:
https://reviews.llvm.org/D98265
https://reviews.llvm.org/rG8dc70bdcd0fe4efb65876dce0144d9c3386a2f07

But in order to prevent further building issues related to the usage of it in other compilation unit, this patch adjusts the llvm.h header as a workaround instead.

We will further investigate the real problem of build compiler as a follow-up.

Diff Detail

Event Timeline

Xiangling_L created this revision.Mar 12 2021, 1:37 PM
Xiangling_L requested review of this revision.Mar 12 2021, 1:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 1:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jsji added a comment.Mar 12 2021, 1:41 PM

If we use this workaround, can we also revert 561fb7f60ab631e712c3fb6bbeb47061222c6818 and 8dc70bdcd0fe4efb65876dce0144d9c3386a2f07 in this patch?

RKSimon added inline comments.
clang/include/clang/Basic/LLVM.h
25

Is there a better comment? "unblock the XL build compiler issue" isn't going to mean much to someone in the future....

38

Should these 2 forward declarations be removed?

If we use this workaround, can we also revert 561fb7f60ab631e712c3fb6bbeb47061222c6818 and 8dc70bdcd0fe4efb65876dce0144d9c3386a2f07 in this patch?

Good idea. Will do

Xiangling_L marked 2 inline comments as done.Mar 12 2021, 1:52 PM
Xiangling_L added inline comments.
clang/include/clang/Basic/LLVM.h
25

How about "Add this header as a workaround to prevent "too few template arguments for class template 'SmallVector' ""? Or do you have any suggestion?

38

Thanks. will do.

Xiangling_L edited the summary of this revision. (Show Details)Mar 12 2021, 1:58 PM
Xiangling_L marked 2 inline comments as done.

Addressed the comments;

Thanks @Xiangling_L and also to the other reviewers for the great comments. This LGTM.

This revision is now accepted and ready to land.Mar 12 2021, 2:24 PM
Xiangling_L added inline comments.Mar 12 2021, 3:13 PM
clang/include/clang/Basic/LLVM.h
71

It's my bad to not wait for a build finished before updating the patch, I saw a lot of build failures as the following that seems related to the deletion here:

/data/xling/wyvern/master-external/llvm-project/clang/include/clang/Basic/Module.h:222:3: error: no template named 'SmallVector'; did you mean 'llvm::SmallVector'?
  SmallVector<UnresolvedHeaderDirective, 1> UnresolvedHeaders;
  ^~~~~~~~~~~
  llvm::SmallVector
/data/xling/wyvern/master-external/llvm-project/clang/include/clang/Basic/LLVM.h:38:42: note: 'llvm::SmallVector' declared here
  template<typename T, unsigned N> class SmallVector;
                                         ^

I will update the patch accordingly after I have a successful build and run the test.

Harbormaster completed remote builds in B93602: Diff 330369.

Bring back two forward declarations to avoid changing pervasive SmallVector to llvm::SmallVector

Retained using declarations look fine. LGTM.