[PATCH] D46441: [clang][CodeGenCXX] Noalias attr for copy/move constructor arguments

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

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

The code you're generating is not correct. The object being constructed cannot be accessed through a pointer not derived from `this`, but for a copy or move constructor, it is perfectly legitimate for the //source// of the construction to be aliased. For example:

  extern struct A a;
  struct A {
    int m, n;
    A(const A &v) : m(v.m), n((a.m = 1, v.m)) {}
  };

It would be incorrect to optimize this constructor to

  A(const A &v) : m(v.m), n(v.m) { a.m = 1; }

by deducing that `v` cannot alias `::a`, but it appears that is exactly what your patch will allow.

So: you should mark the `this` parameter as `noalias`, not the formal parameter of the function. (And as Eli notes, you should not treat the copy / move constructors as being special cases.)

However, I would question whether this change is correct at all. Note that the standard wording says "the value of the object or subobject thus obtained is unspecified" not "the behavior is undefined", and as far as I can tell, a violation of `noalias` results in undefined behavior in LLVM IR, not merely an unspecified value. (For what it's worth, I think this standard text is in error and UB would be appropriate, but we should at least discuss whether we want to take this additional liberty not technically granted by the standard.)



================
Comment at: lib/CodeGen/CodeGenModule.cpp:2513
+      cast<CXXConstructorDecl>(D)->isCopyOrMoveConstructor())
+    F->addParamAttr(1, llvm::Attribute::NoAlias);
+
----------------
It's not strictly correct to assume that you know the correspondence between `llvm::Function` parameter indices and those of the `clang::FunctionDecl` like this. That's up to the ABI, and for constructors in particular, there may be a VTT parameter to deal with (and `__attribute__((pass_object_size))` can also interfere). You can probably get away with assuming that the `this` parameter has index 0 for now, but even that is likely to break at some point down the line.


Repository:
  rC Clang

https://reviews.llvm.org/D46441