[PATCH] D102153: [SelectionDAG] Fix argument copy elision with irregular types

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thinking about this more, I think this is a good change. Currently we use the zext and sext attributes to document if the high bits of an argument are initialized, and if so, to what. If those attributes aren't present, we shouldn't do copy elision. I have a minor concern that needs to be addressed, though.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9824
-        DL.getTypeStoreSize(Arg->getType()) !=
-            DL.getTypeAllocSize(AI->getAllocatedType()) ||
         ArgCopyElisionCandidates.count(Arg)) {
----------------
Don't we still need this check? I don't see any other code that checks that the allocated type and argument type match. Otherwise we could try to copy elide an i32 stored into an i64 alloca, for example. Please add a test for that, sorry for not already having one.


================
Comment at: llvm/test/CodeGen/X86/arg-copy-elide.ll:80
+; CHECK-NEXT:    andb $1, %al
+; CHECK-NEXT:    movb %al, {{[0-9]+}}(%esp)
 ; CHECK-NEXT:    leal {{[0-9]+}}(%esp), %eax
----------------
rnk wrote:
> We don't want this, do we? Seems like it would be a regression.
This code turns out to not be representative of what clang generates, so I don't think we need to worry about it. See this bool example here:
https://gcc.godbolt.org/z/efPavanfb
```
void escape(bool *);
void bool_arg(bool b) { escape(&b); }
```
We fail to copy elide because the argument is i1, but the alloca is i8. I think this will be true generally for non-byte-sized integers, so there is really no harm to this change to copy elision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102153