[PATCH] D55510: [ExprConstant] Improve memchr/memcmp for type mismatch and multibyte element types

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/AST/ExprConstant.cpp:6147-6148
+      return ZeroInitialization(E);
+    if (!Result.checkNullPointerForFoldAccess(Info, E, AK_Read))
+      return false;
+    QualType CharTy =
----------------
Why do we need to do this explicitly, rather than allowing to simply happen as part of the first `handleLValueToRValueConversion` below?


================
Comment at: lib/AST/ExprConstant.cpp:6150
+    QualType CharTy =
+        Info.Ctx.getBaseElementType(getType(Result.getLValueBase()));
+    const bool IsRawByte = BuiltinOp == Builtin::BImemchr ||
----------------
Considering the LValue base here is incorrect. The object or array we're copying could be a subobject, and the complete object type (the type of the lvalue base) is irrelevant for such copies. Use `Result.Designator.getType(Info.Ctx)` to find the type that the lvalue designates. (You can bail out here if the designator is invalid; `handleLValueToRValueConversion` will always fail on such cases anyway.


================
Comment at: lib/AST/ExprConstant.cpp:6159-6160
+    // Give up on byte-oriented matching against multibyte elements.
+    if (IsRawByte && Info.Ctx.getTypeSize(CharTy) > Info.Ctx.getCharWidth())
+      return false;
     // Figure out what value we're actually looking for (after converting to
----------------
Please add an assert here that the types do match in the non-raw-byte case. (They must, or the subobject designator would be invalid; the only special case is that a `void*` can point to anything, and only the `memchr` variants here take a `void*`.)


================
Comment at: lib/AST/ExprConstant.cpp:8433-8436
+    QualType CharTy1 =
+        Info.Ctx.getBaseElementType(getType(String1.getLValueBase()));
+    QualType CharTy2 =
+        Info.Ctx.getBaseElementType(getType(String2.getLValueBase()));
----------------
Per above comment, this is not correct.


================
Comment at: lib/AST/ExprConstant.cpp:8467
+      assert(MaxLength);
+      for (;;) {
+        APValue Char1, Char2;
----------------
Style nit: please use `while (true)` rather than `for (;;)`


================
Comment at: lib/AST/ExprConstant.cpp:8471-8476
+        // We have compatible in-memory widths, but a possible type and internal
+        // representation mismatch. Assuming two's complement representation,
+        // including 0 for `false` and 1 for `true`, we can check an appropriate
+        // number of elements for equality even if they are not byte-sized.
+        const APSInt Char1InMem = Char1.getInt().extOrTrunc(CharTy1Width);
+        const APSInt Char2InMem = Char2.getInt().extOrTrunc(CharTy1Width);
----------------
The *only* possible problem here is for `bool`, right? This comment would be clearer if it said that.


================
Comment at: lib/AST/ExprConstant.cpp:8484
+          }
+          return false;
+        }
----------------
Please add a comment before this return indicating that the result depends on byte order (and maybe a FIXME to compare the bytes in the right order rather than bailing out). We're going to need to deal with the byte order / representation problem for `std::bit_cast` pretty soon anyway.


================
Comment at: lib/AST/ExprConstant.cpp:8488-8489
+          return false;
+        if (MaxLength <= LengthPerElement)
+          break;
+        MaxLength -= LengthPerElement;
----------------
This looks wrong. If `0 < MaxLength && MaxLength < LengthPerElement`, we're supposed to compare *part of* the next element; this current approach doesn't do that. (If you did one more full-element compare, that'd be conservatively correct because we're only checking for equality, not ordering, but perhaps we should only permit cases where `MaxLength` is a multiple of `LengthPerElement`?


================
Comment at: test/CodeGenCXX/builtins.cpp:41
+    matchedFirstByteIn04030201();
+    // CHECK: call void @_ZN27MemchrMultibyteElementTests26matchedFirstByteIn04030201Ev()
+  }
----------------
Please test this in a way that doesn't rely on IR generation constant-evaluating `if` conditions. Moreover, there's nothing about IR generation that you're actually testing here, so please phrase this as a `Sema` test instead (eg, check that a VLA bound gets folded to a constant). Likewise for the other tests below.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55510/new/

https://reviews.llvm.org/D55510