[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:8465
+        return false;
+      uint64_t LengthPerElement{CharTy1Width / Info.Ctx.getCharWidth()};
+      assert(MaxLength && "MaxLength should not be zero: the following loop "
----------------
Use `toCharUnitsFromBits` here, and use `CharUnits` as a type-safe way of representing a size in terms of a number of `char`s. In fact, consider using `getTypeSizeInChars` a few lines above and only operating in `char`s, never bits.


================
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);
----------------
hubert.reinterpretcast wrote:
> 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`.
There is no type other than `bool` that needs the `extOrTrunc`; maybe drop the "especially"?


================
Comment at: lib/AST/ExprConstant.cpp:8488-8489
+          return false;
+        if (MaxLength <= LengthPerElement)
+          break;
+        MaxLength -= LengthPerElement;
----------------
hubert.reinterpretcast wrote:
> 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.
Oh, I see now. Right. :)

Maybe add "Be careful not to compare more than MaxLength bytes" to the FIXME above, so a future maintainer doesn't get confused?


================
Comment at: test/CodeGenCXX/builtins.cpp:41
+    matchedFirstByteIn04030201();
+    // CHECK: call void @_ZN27MemchrMultibyteElementTests26matchedFirstByteIn04030201Ev()
+  }
----------------
hubert.reinterpretcast wrote:
> 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.
Seems fine to have a test that we don't fold certain cases at all, with a comment saying that it's OK if we start folding them to a constant and what that constant should be.

Generally the principle is that we want our unit tests to test as few layers of the stack as possible; the chosen test directory should ideally correspond to the layer under test. If you'd really like this to be more of an integration test, that's fine too (eg, if you have code like this in a platform header that really must be handled a certain way), but then it would belong in `test/Integration`. If you do put it in `test/Integration`, it'd then be reasonable to include -O in the test flags too, if that makes sense for what you want to test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55510