[clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[dsanders]

Summary

This clang-tidy check is looking for unsigned integer variables whose initializer
starts with an implicit cast from llvm::Register and changes the type of the
variable to llvm::Register (dropping the llvm:: where possible).

[Eugene.Zelenko]

I think patch should be split at least on Clang-tidy check and results of its run on LLVM code. Probably per-target patches is even better solution.

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

49

You could use const auto * for iterators.

clang-tools-extra/docs/ReleaseNotes.rst

85

Please use double back-ticks to highlight language constructs. Same in documentation.

[dsanders]

UsingDirectiveDecl -> auto
-> `
Split LLVM changes into another patch

[dsanders]

In D65919#1621477, @Eugene.Zelenko wrote:

I think patch should be split at least on Clang-tidy check and results of its run on LLVM code. Probably per-target patches is even better solution.

I've split the LLVM changes out into D65962. I expect it will be split up further for commit purposes if nothing else but I've kept it in a single review for now.

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

40

How about variable %0 declared as %1; use '%2' instead and move it below to where the fix-it is issued?

44

Formatting looks off here, you should run the patch through clang-format.

53

I don't think you should issue a second diagnostic on the same line. Instead, only issue the previous diagnostic with the fixit attached to it.

[dsanders]

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

53

I don't mind changing this but I thought I should mention that I'm following the example set by the code generated by add_new_check.py which has the diagnostic separate from the note with the fixit:

diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
    << MatchedDecl;
diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
    << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");

Is the example doing it the right way?

[dsanders]

Changed diagnostic message and merged the fixit into the original diagnostic
Made the variable names more distinct in the tests
Added a test to cover using namespace llvm in the global namespace
Slight correction to the iteration over the contexts to avoid skipping one
clang-format

[dsanders]

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

48–51

There's something not quite right here but I haven't figured out what yet. One of the tests (apply_2()) is failing (which is odd because I ran ninja check-clang-tools before posting the patch) because the using namespace llvm in the function scope doesn't show up in the DeclContext for the function. Am I looking in the wrong place for it?

More generally, I suspect there's an easier way to omit the llvm:: than the way I'm using on lines 43-52.

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

53

That script is intended to generate boilerplate so that you don't have to and to show a very minimal example of how to print output. So it's both correct and not particularly helpful for real checks at the same time, if that makes sense.

[dsanders]

• Give in on eliding the llvm:: for using namespace llvm inside a function They don't appear in the DeclContext and I've been unable to find a way to reasonably detect them Fortunately they don't occur in practice

[dsanders]

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

48–51

I've given in on trying to elide the llvm:: for this case as the using namespace llvm inside a function doesn't seem to be easily discoverable and doesn't occur in practice

53

I guess it makes sense to have one example of a warning and one of a note. It might be worth adding a comment suggesting to those new to clang-tidy that they should try to emit a single 'this is wrong; do this' diagnostic rather than the two separate ones from the example but that's not for this patch at least.

Thanks

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

28–29

Please keep this list sorted in alphabetical order.

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

22

Should be ::llvm::Register but not hugely important as I doubt we care too terribly much about inner namespaces named llvm.

32

You don't seem to be using the operatorDecl binding -- you can remove it, if it's not intended to be used.

46

This is a fairly expensive operation compared to calling getName(); are there situations where you think you need the extra work to be done? (Same comment below.)

53

Yeah, I wouldn't be opposed to a patch to the python script to make that more clear. Or dropping the note entirely under the assumption the user can look at the diag() signature to see how to make warnings vs errors vs notes.

clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst

11

initialize -> initialization

clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp

61

I'd also like to see some tests like:

void func(unsigned Reg);

struct S {
  unsigned Reg;

  S(unsigned Reg) : Reg(Reg) {}
};

void other_func() {
  func(getReg());
  S s{getReg()};
  s.Reg = getReg();
}

[dsanders]

• Correct alphabetical list in LLVMTidyModule.cpp and prevent add_new_check.py getting it wrong in future
• Lookup Register class via ::llvm::Register instead of llvm::Register
• Removed unnecessary bindings
• Typo in the docs
• More tests

Haven't made the change to avoid getQualifiedNameAsString() yet in case there's an easy way to tell
if the llvm namespace found is inside another namespace.

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

28–29

This change was made by add_new_check.py and looking at the code, it's been confused by the readability::. I'll move it to the right place and add a using readability::NamespaceCommentCheck so we can drop the readability:: and have add_new_check.py get it right in future.

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

46

None that are likely to occur. getName() with some way to check that there's no enclosing namespace would be equivalent. Is there a cheap way to tell if a namespace is at the top level?

clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst

11

It was meant to be 'initializer'. I'll correct that

clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp

61

Added tests for the following cases that do not make changes*

1. Similar interface but not called Register
2. Register class not inside llvm namespace
3. Calling a function that takes an llvm::Register with a llvm::Register argument
4. Calling a function that takes an unsigned and is given an llvm::Register argument
5. Initializing an llvm::Register using an llvm::Register argument
6. Initializing any other object using a constructor parameter that's unsigned and a llvm::Register argument
7. Assigning to a member of llvm::Register
8. Assigning to a unsigned member of any other object

*4, 6, and 8 should eventually but don't yet

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

26

I would rather use the fully-qualified names below -- the namespaces are actually of interest when needing to see what checks rely on what other modules quickly.

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

46

You could check if its DeclContext is the translation unit, I believe.

[dsanders]

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

26

In that case I think I need some more detail on the alphabetical order I need to preserve. Do the namespaces factor into the order? If so, then PreferIsaOrDynCastInConditionalsCheck was in the wrong place prior to this patch and add_new_check.py is behaving correctly. If not, then the existing order was correct but add_new_check.py is inserting new lines incorrectly

[aaron.ballman]

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

26

We usually alphabetize based on the string literal for the check being registered. This usually results in the list also being sorted by the check implementation class name, but not always.

[dsanders]

• registerCheck<> order

[aaron.ballman]

Aside from the few remaining nits, I think this is almost ready to go.

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

36–37

Spurious change that moved llvm-namespace-comment to be out of alphabetical order?

clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp

61

Nice, thank you!

[dsanders]

• Sort order again :-)
• Avoid getQualifiedNameAsString() in favour of getName() and checking

[dsanders]

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

36–37

Oops, not sure what happened there. I think I rolled it back to one of the earlier revisions somehow

[aaron.ballman]

LGTM aside from a nit with auto.

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

48

Don't use const auto * here as the type is not spelled out in the initializer.

[dsanders]

r370512

clang-tools-extra/clang-tidy/llvm/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyLLVMModule
   HeaderGuardCheck.cpp
   IncludeOrderCheck.cpp
   LLVMTidyModule.cpp
   PreferIsaOrDynCastInConditionalsCheck.cpp
+  PreferRegisterOverUnsignedCheck.cpp
   TwineLocalCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyReadabilityModule
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

 //===--- LLVMTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../readability/NamespaceCommentCheck.h"
 #include "HeaderGuardCheck.h"
 #include "IncludeOrderCheck.h"
 #include "PreferIsaOrDynCastInConditionalsCheck.h"
+#include "PreferRegisterOverUnsignedCheck.h"
 #include "TwineLocalCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm_check {
 
 class LLVMModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<LLVMHeaderGuardCheck>("llvm-header-guard");
     CheckFactories.registerCheck<IncludeOrderCheck>("llvm-include-order");
+    CheckFactories.registerCheck<PreferRegisterOverUnsignedCheck>(
+        "llvm-prefer-register-over-unsigned");
     CheckFactories.registerCheck<readability::NamespaceCommentCheck>(
         "llvm-namespace-comment");
     CheckFactories.registerCheck<PreferIsaOrDynCastInConditionalsCheck>(
         "llvm-prefer-isa-or-dyn-cast-in-conditionals");
     CheckFactories.registerCheck<TwineLocalCheck>("llvm-twine-local");
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<LLVMModule> X("llvm-module",
                                                   "Adds LLVM lint checks.");
 
 } // namespace llvm_check
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the LLVMModule.
 volatile int LLVMModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h

+//===--- PreferRegisterOverUnsignedCheck.h - clang-tidy ---------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_PREFERREGISTEROVERUNSIGNEDCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_PREFERREGISTEROVERUNSIGNEDCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace llvm_check {
+
+/// Historically, LLVM has used `unsigned` to represent registers. Since then
+/// a `Register` object has been introduced for improved type-safety and make
+/// the code more explicit.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/llvm-prefer-register-over-unsigned.html
+class PreferRegisterOverUnsignedCheck : public ClangTidyCheck {
+public:
+  PreferRegisterOverUnsignedCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace llvm_check
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_PREFERREGISTEROVERUNSIGNEDCHECK_H

clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp

+//===--- PreferRegisterOverUnsignedCheck.cpp - clang-tidy -----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "PreferRegisterOverUnsignedCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace llvm_check {
+
+void PreferRegisterOverUnsignedCheck::registerMatchers(MatchFinder *Finder) {
+  auto RegisterClassMatch = hasType(
+      cxxRecordDecl(hasName("llvm::Register")).bind("registerClassDecl"));
+
+  Finder->addMatcher(
+      valueDecl(allOf(
+          hasType(qualType(isUnsignedInteger()).bind("varType")),
+          varDecl(
+              hasInitializer(exprWithCleanups(
+                  has(implicitCastExpr(has(cxxMemberCallExpr(allOf(
+                      on(RegisterClassMatch),
+                      has(memberExpr(hasDeclaration(
+                          cxxConversionDecl().bind("operatorDecl"))))))))))))
+              .bind("var"))),
+      this);
+}
+
+void PreferRegisterOverUnsignedCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *VarType = Result.Nodes.getNodeAs<QualType>("varType");
+  const auto *UserVarDecl = Result.Nodes.getNodeAs<VarDecl>("var");
+
+  diag(UserVarDecl->getLocation(), "var %0 is %1 but holds a register")
+      << UserVarDecl << *VarType;
+  StringRef Replacement = "llvm::Register";
+  const DeclContext *Context = UserVarDecl->getDeclContext();
+  while((Context = Context->getParent()) != nullptr) {
+    if (const auto *Namespace = dyn_cast<NamespaceDecl>(Context))
+      if (Namespace->getQualifiedNameAsString() == "llvm")
+        Replacement = "Register";
+    for (const auto *UsingDirective: Context->using_directives())
+      if (UsingDirective->getNominatedNamespace()
+              ->getQualifiedNameAsString() == "llvm")
+        Replacement = "Register";
+  }
+  diag(UserVarDecl->getLocation(), "use '%0'", DiagnosticIDs::Note)
+      << Replacement
+      << FixItHint::CreateReplacement(
+             UserVarDecl->getTypeSourceInfo()->getTypeLoc().getSourceRange(),
+             Replacement);
+}
+
+} // namespace llvm_check
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`linuxkernel-must-use-errs
   ``linux/err.h``.
 
 - New :doc:`google-upgrade-googletest-case
   <clang-tidy/checks/google-upgrade-googletest-case>` check.
 
   Finds uses of deprecated Googletest APIs with names containing ``case`` and
   replaces them with equivalent APIs with ``suite``.
 
+- New :doc:`llvm-prefer-register-over-unsigned
+  <clang-tidy/checks/llvm-prefer-register-over-unsigned>` check.
+
+  Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
+  them to use ``Register``
+
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. toctree::
    hicpp-use-nullptr (redirects to modernize-use-nullptr) <hicpp-use-nullptr>
    hicpp-use-override (redirects to modernize-use-override) <hicpp-use-override>
    hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) <hicpp-vararg>
    linuxkernel-must-use-errs
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-prefer-isa-or-dyn-cast-in-conditionals
+   llvm-prefer-register-over-unsigned
    llvm-twine-local
    misc-definitions-in-headers
    misc-misplaced-const
    misc-new-delete-overloads
    misc-non-copyable-objects
    misc-non-private-member-variables-in-classes
    misc-redundant-expression
    misc-static-assert

clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst

+.. title:: clang-tidy - llvm-prefer-register-over-unsigned
+
+llvm-prefer-register-over-unsigned
+==================================
+
+Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
+them to use ``Register``.
+
+Currently this works by finding all variables of unsigned integer type whose
+initialize begins with an implicit cast from ``Register`` to ``unsigned``.
+
+.. code-block:: c++
+
+  void example(MachineOperand &MO) {
+    unsigned Reg = MO.getReg();
+    ...
+  }
+
+becomes:
+
+.. code-block:: c++
+
+  void example(MachineOperand &MO) {
+    Register Reg = MO.getReg();
+    ...
+  }
+

clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp

+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+void apply_1() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: var 'Reg' is 'unsigned int' but holds a register [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
+
+void apply_2() {
+  using namespace llvm;
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: var 'Reg' is 'unsigned int' but holds a register [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+namespace llvm {
+void apply_3() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: var 'Reg' is 'unsigned int' but holds a register [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_3()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+} // end namespace llvm
+
+void done_1() {
+  llvm::Register Reg = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
+
+void done_2() {
+  using namespace llvm;
+  Register Reg = getReg();
+  // CHECK-FIXES: done_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+namespace llvm {
+void done_3() {
+  Register Reg = getReg();
+  // CHECK-FIXES: done_3()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+} // end namespace llvm