rogfer01 added inline comments.

================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:90
   union {
-    unsigned DirectOffset;     // isDirect() || isExtend()
-    unsigned IndirectAlign;    // isIndirect()
-    unsigned AllocaFieldIndex; // isInAlloca()
+    llvm::StructType *ExtendSeq; // isCoerceAndExpand()
+    unsigned DirectOffset;       // isDirect() || isExtend()
----------------
rjmccall wrote:
> Hmm.  I understand the need to use something uniqued here, but I think it 
> would probably be more natural to at least use a `llvm::ConstantDataArray` 
> (which could be null if there are no interesting bits) instead of encoding 
> the information into the types of a struct type.  That would also make it 
> easy to generalize the elements to an arbitrary flags type.
> 
> Also, on 64-bit targets this will increase the size of an `ABIArgInfo` to 
> four pointers from three.  That's fine to the extent that we work with 
> independent `ABIArgInfo`s, but I'm getting a little annoyed at the storage 
> overhead of the array of `ABIArgInfo`s in `CGFunctionInfo` given that, in the 
> overwhelmingly common case, an `ABIArgInfo` is no more than a kind and maybe 
> a few of these flags.  Maybe there's some reasonable way to optimize the 
> storage of an `ABIArgInfo` in a `CGFunctionInfo` so that we only need the 
> extra storage in less-common cases?  Like extracting out a base class that's 
> the Kind+Flags and making the main array be an array of those + an optional 
> index into a second trailing array of full `ABIArgInfo`s.
> 
> I might be overthinking this, though.
Oh, of course, using a value rather than a type is more sensible. I will change 
this.

The idea of having a lighter `ABIArgInfo` and an ancillary extra array for it 
seems sensible. I guess it may depend on how early/late we know the precise 
kind of `ABIArgInfo` we need to represent. I haven't looked at this yet, I'll 
investigate.


================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:475
 
+static_assert(std::is_trivially_copyable<ABIArgInfo>::value,
+    "ABIArgInfo must be trivially copyable as it is embedded as trailing "
----------------
rogfer01 wrote:
> I think this is the right trait here. I spent too much time debugging this :)
But today I realised that GCC 4.9 does not implement this so it will have to go 
away :(


https://reviews.llvm.org/D48589



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to