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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/IR/Attributes.cpp:554
+      if (getKindAsEnum() == AI.getKindAsEnum())
+        return getValueAsType() < AI.getValueAsType();
+      return getKindAsEnum() < AI.getKindAsEnum();
----------------
dexonsmith wrote:
> t.p.northover wrote:
> > 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.
> I would prefer `llvm_unreachable` or `report_fatal_error` with a comment saying that comparing on type wouldn't be stable, but is also not possible.  Or, sorting it in an unstable way with comment saying it's illegal, and adding a corresponding verifier error.  Happy with whatever you and David decide though, if you think it's better as is.
Yeah, +1 to that - if the code isn't used, unreachable/assert - then we can discover if it ever gets used and add test coverage for it.


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

https://reviews.llvm.org/D62319