[PATCH] D65234: [CodeGen]: don't treat structures returned in registers as memory inputs

[Alexander Potapenko via Phabricator via cfe-commits]

glider marked 2 inline comments as done.
glider added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:1989
   std::vector<llvm::Value*> Args;
+  std::vector<bool> ResultTypeRequiresCast;
 
----------------
nickdesaulniers wrote:
> Are we able to use something other than `std::vector<bool>` here from ADT?
> http://llvm.org/docs/ProgrammersManual.html#bit-storage-containers-bitvector-sparsebitvector
I don't think `std::vector<bool>` is a bottleneck here, but since it might be deprecated someday let's just not use it.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:2287
   assert(RegResults.size() == ResultRegDests.size());
+  assert(ResultTypeRequiresCast.size() <= ResultRegDests.size());
   for (unsigned i = 0, e = RegResults.size(); i != e; ++i) {
----------------
efriedma wrote:
> Not "=="?
Turns out ResultRegDests can be also extended by `addReturnRegisterOutputs` above (line 2122), so no.
I've added a comment to clarify that.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:2325
+      Dest = MakeAddrLValue(
+          A, getContext().getIntTypeForBitwidth(Size, /*Signed*/ false));
+    }
----------------
efriedma wrote:
> Will this work if the struct is an unusual size, like `sizeof(struct s) == 3` or `sizeof(struct s) == 32`?  (3 is unlikely to show up in real code, but 32 could correspond to a vector register.)
For a 3-byte struct this works as follows: a 3-byte structure is allocated on the stack with alignment 2, then the assembly returns a 4-byte integer that's written to that 3-byte structure.
This conforms to what GCC does, and I think it's legal to let the assembly write more than a structure size here, as the corresponding register size is bigger (i.e. it's a bug in the C code, not Clang).

For a 32-bit struct we crash now, whereas GCC reports an "impossible constraint in ‘asm’"
It's interesting that the crash happens in the frontend, i.e. it's somehow knows the maximum possible size for the register operand.
We can check that `getContext().getIntTypeForBitwidth(Size, /*Signed*/ false)` is a nonnull type to prevent the crashes.


================
Comment at: clang/test/CodeGen/PR42672.c:50
+  struct {
+    long long int v1, v2, v3, v4;
+  } str;
----------------
The acceptable size actually depends on the target platform. Not sure we can test for all of them, and we'll probably need to restrict this test to e.g. x86


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65234