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

Hubert Tong via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 10 14:45:13 PST 2018


hubert.reinterpretcast added inline comments.


================
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
----------------
rsmith wrote:
> 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*`.)
Will do.


================
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);
----------------
rsmith wrote:
> The *only* possible problem here is for `bool`, right? This comment would be clearer if it said that.
We do assume two's complement representation (which C89 does not require to be true); so, `bool` is more problematic than other things, but the comment is not just about `bool`.


================
Comment at: lib/AST/ExprConstant.cpp:8488-8489
+          return false;
+        if (MaxLength <= LengthPerElement)
+          break;
+        MaxLength -= LengthPerElement;
----------------
rsmith wrote:
> 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`?
We //are// doing one more full-element compare to be conservatively correct. There is a test for that.


================
Comment at: test/CodeGenCXX/builtins.cpp:41
+    matchedFirstByteIn04030201();
+    // CHECK: call void @_ZN27MemchrMultibyteElementTests26matchedFirstByteIn04030201Ev()
+  }
----------------
rsmith wrote:
> 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.
Not all of these cases can fold successfully with the current implementation. The check would need to be that we either fold (and get the right value), or don't fold at all.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55510





More information about the cfe-commits mailing list