[torque] Warn if bitfields are checked with && rather than &

We can do a good job of optimizing Torque expressions that load and
check multiple bitfields from a bitfield struct, but only if those
expressions are written using the binary `&` operator as opposed to the
logical `&&`. This change adds a lint rule to detect some simple cases
where we should clearly prefer `&` to `&&`.

Bug: v8:7793
Change-Id: Id996a7971cff8f7f83198075a172170d9c7d42e9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2207666
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67991}

src/torque/implementation-visitor.cc

@@ -731,8 +731,17 @@ VisitResult ImplementationVisitor::Visit(LogicalAndExpression* expr) {
   VisitResult true_result;
   {
     StackScope true_block_scope(this);
+    VisitResult right_result = Visit(expr->right);
+    if (TryGetSourceForBitfieldExpression(expr->left) != nullptr &&
+        TryGetSourceForBitfieldExpression(expr->right) != nullptr &&
+        TryGetSourceForBitfieldExpression(expr->left)->value ==
+            TryGetSourceForBitfieldExpression(expr->right)->value) {
+      Lint(
+          "Please use & rather than && when checking multiple bitfield "
+          "values, to avoid complexity in generated code.");
+    }
     true_result = true_block_scope.Yield(
-        GenerateImplicitConvert(TypeOracle::GetBoolType(), Visit(expr->right)));
+        GenerateImplicitConvert(TypeOracle::GetBoolType(), right_result));
   }
   assembler().Goto(done_block);
 
@@ -833,6 +842,17 @@ VisitResult ImplementationVisitor::Visit(LocationExpression* expr) {
   return scope.Yield(GenerateFetchFromLocation(GetLocationReference(expr)));
 }
 
+VisitResult ImplementationVisitor::Visit(FieldAccessExpression* expr) {
+  StackScope scope(this);
+  LocationReference location = GetLocationReference(expr);
+  if (location.IsBitFieldAccess()) {
+    if (auto* identifier = IdentifierExpression::DynamicCast(expr->object)) {
+      bitfield_expressions_[expr] = identifier->name;
+    }
+  }
+  return scope.Yield(GenerateFetchFromLocation(location));
+}
+
 const Type* ImplementationVisitor::Visit(GotoStatement* stmt) {
   Binding<LocalLabel>* label = LookupLabel(stmt->label->value);
   size_t parameter_count = label->parameter_types.size();
@@ -2682,6 +2702,16 @@ VisitResult ImplementationVisitor::Visit(CallExpression* expr,
       LanguageServerData::AddDefinition(expr->callee->name->pos,
                                         callable->IdentifierPosition());
     }
+    if (expr->callee->name->value == "!" && arguments.parameters.size() == 1) {
+      PropagateBitfieldMark(expr->arguments[0], expr);
+    }
+    if (expr->callee->name->value == "==" && arguments.parameters.size() == 2) {
+      if (arguments.parameters[0].type()->IsConstexpr()) {
+        PropagateBitfieldMark(expr->arguments[1], expr);
+      } else if (arguments.parameters[1].type()->IsConstexpr()) {
+        PropagateBitfieldMark(expr->arguments[0], expr);
+      }
+    }
     return scope.Yield(
         GenerateCall(name, arguments, specialization_types, is_tailcall));
   }

src/torque/implementation-visitor.h

@@ -498,6 +498,7 @@ class ImplementationVisitor {
   VisitResult GetBuiltinCode(Builtin* builtin);
 
   VisitResult Visit(LocationExpression* expr);
+  VisitResult Visit(FieldAccessExpression* expr);
 
   void VisitAllDeclarables();
   void Visit(Declarable* delarable);
@@ -783,9 +784,32 @@ class ImplementationVisitor {
     ReplaceFileContentsIfDifferent(file, content);
   }
 
+  const Identifier* TryGetSourceForBitfieldExpression(
+      const Expression* expr) const {
+    auto it = bitfield_expressions_.find(expr);
+    if (it == bitfield_expressions_.end()) return nullptr;
+    return it->second;
+  }
+
+  void PropagateBitfieldMark(const Expression* original,
+                             const Expression* derived) {
+    if (const Identifier* source =
+            TryGetSourceForBitfieldExpression(original)) {
+      bitfield_expressions_[derived] = source;
+    }
+  }
+
   base::Optional<CfgAssembler> assembler_;
   NullOStream null_stream_;
   bool is_dry_run_;
+
+  // Just for allowing us to emit warnings. After visiting an Expression, if
+  // that Expression is a bitfield load, plus an optional inversion or an
+  // equality check with a constant, then that Expression will be present in
+  // this map. The Identifier associated is the bitfield struct that contains
+  // the value to load.
+  std::unordered_map<const Expression*, const Identifier*>
+      bitfield_expressions_;
 };
 
 void ReportAllUnusedMacros();

test/unittests/torque/torque-unittest.cc

@@ -87,6 +87,8 @@ struct float64_or_hole {
 }
 
 extern operator '+' macro IntPtrAdd(intptr, intptr): intptr;
+extern operator '!' macro Word32BinaryNot(bool): bool;
+extern operator '==' macro Word32Equal(int32, int32): bool;
 
 intrinsic %FromConstexpr<To: type, From: type>(b: From): To;
 intrinsic %RawDownCast<To: type, From: type>(x: From): To;
@@ -113,6 +115,13 @@ FromConstexpr<intptr, constexpr int31>(i: constexpr int31): intptr {
 FromConstexpr<intptr, constexpr intptr>(i: constexpr intptr): intptr {
   return %FromConstexpr<intptr>(i);
 }
+extern macro BoolConstant(constexpr bool): bool;
+FromConstexpr<bool, constexpr bool>(b: constexpr bool): bool {
+  return BoolConstant(b);
+}
+FromConstexpr<int32, constexpr int31>(i: constexpr int31): int32 {
+  return %FromConstexpr<int32>(i);
+}
 
 macro Cast<A : type extends Object>(implicit context: Context)(o: Object): A
     labels CastError {
@@ -801,6 +810,25 @@ TEST(Torque, CatchFirstHandler) {
           "catch handler always has to be first, before any label handler"));
 }
 
+TEST(Torque, BitFieldLogicalAnd) {
+  std::string prelude = R"(
+    bitfield struct S extends uint32 {
+      a: bool: 1 bit;
+      b: bool: 1 bit;
+      c: int32: 5 bit;
+    }
+    macro Test(s: S): bool { return
+  )";
+  std::string postlude = ";}";
+  std::string message = "use & rather than &&";
+  ExpectFailingCompilation(prelude + "s.a && s.b" + postlude,
+                           HasSubstr(message));
+  ExpectFailingCompilation(prelude + "s.a && !s.b" + postlude,
+                           HasSubstr(message));
+  ExpectFailingCompilation(prelude + "!s.b && s.c == 34" + postlude,
+                           HasSubstr(message));
+}
+
 }  // namespace torque
 }  // namespace internal
 }  // namespace v8