[PATCH] D62319: IR: add 'byval(<ty>)' variant to 'byval' function parameters

[Tim Northover via Phabricator via llvm-commits]

t.p.northover marked 4 inline comments as done.
t.p.northover added a comment.

Some replies, and I'll upload a new revision tomorrow with the things that definitely need fixing.



================
Comment at: llvm/include/llvm/IR/Argument.h:81
 
+  /// If this is a byval argument, return its size.
+  Type *getParamByValType() const;
----------------
dblaikie wrote:
> Comment looks to be out of date (speaks of returning the size, rather than type)
Bother, I did a specific "git diff" run to catch those!


================
Comment at: llvm/lib/IR/Attributes.cpp:394-395
+    Result += "byval";
+    Type *Ty = getValueAsType();
+    if (Ty) {
+      raw_string_ostream OS(Result);
----------------
dblaikie wrote:
> You can roll the declaration into the condition if you like - pretty common in the LLVM codebase, though not a stylistic requirement by any means.
Yep. No idea why I didn't do that.


================
Comment at: llvm/lib/IR/Attributes.cpp:554
+      if (getKindAsEnum() == AI.getKindAsEnum())
+        return getValueAsType() < AI.getValueAsType();
+      return getKindAsEnum() < AI.getKindAsEnum();
----------------
dblaikie wrote:
> This being pointer ordering - that means it'll be unstable across runs (which I assume/think none of the other attributes are?) - is that OK?(I don't know if that's OK - it probably is OK)
I did worry about that, but came to the conclusion it doesn't matter unless the same attribute can appear repeatedly. And I don't think that is possible.

Even with IntAttributes, I replaced the corresponding comparison with an `llvm_unreachable` and the only thing that broke was a unit test specifically exercising this function.

So in summary I have no real preference between status quo and `llvm_unreachable`, but don't think it's worth implementing a stable order right now.

I should probably add some checks to that unittest myself though.


================
Comment at: llvm/test/Assembler/invalid-size1.ll:1-4
+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+
+; CHECK: error: expected function name
+declare i8* size 4 @ret_size()
----------------
dblaikie wrote:
> Leftover from the previous version?
Yes. I don't think there's an equivalent (at least not a new one) so I'll remove it.


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

https://reviews.llvm.org/D62319