[PATCH] D98146: OpaquePtr: Turn inalloca into a type attribute

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

> I think byval/sret and the others are close to being able to rip out the code to support the missing type case. A lot of this code is shared with inalloca, so catch this up to the others so that can happen.

I'm a bit confused by the phrasing here - which code supporting the "missing type case" are you referring to? (might be good to have a few more words about which bits of code in particular - though I agree this is the right direction in any case)

Generally looks good to me - but would like @rnk's sign off as well before this is committed.



================
Comment at: llvm/lib/IR/Attributes.cpp:490
     if (Type *Ty = getValueAsType()) {
-      raw_string_ostream OS(Result);
+      // FIXME: This should never be null
       Result += '(';
----------------
Is it? Could you replace this with an assertion instead? 


================
Comment at: llvm/test/Bitcode/compatibility-3.6.ll:408-409
 ; CHECK: declare void @f.param.byval({ i8, i8 }* byval({ i8, i8 }))
-declare void @f.param.inalloca(i8* inalloca)
-; CHECK: declare void @f.param.inalloca(i8* inalloca)
+declare void @f.param.inalloca(i8* inalloca(i8))
+; CHECK: declare void @f.param.inalloca(i8* inalloca(i8))
 declare void @f.param.sret(i8* sret(i8))
----------------
It's confusing to me (though seems to be consistent with the byval change here - but maybe that's a mistake too) why the textual IR in this file is being modified - it has no effect (the textual IR is not used by the test) and the intent is to test compatibility with old IR (which is in the .bc file that is used) - so maybe it's best to leave the text here in a form that matches the old bitcode that the test is running against?

(similarly in the other compatibility tests updated in this patch)


================
Comment at: llvm/test/Bitcode/inalloca-upgrade.test:1-7
+RUN: llvm-dis %p/Inputs/inalloca-upgrade.bc -o - | FileCheck %s
+
+Make sure we upgrade old-style IntAttribute inalloca records to a
+fully typed version correctly.
+
+CHECK: call void @bar({ i32*, i8 }* inalloca({ i32*, i8 }) %ptr)
+CHECK: invoke void @bar({ i32*, i8 }* inalloca({ i32*, i8 }) %ptr)
----------------
Isn't this tested by all the compatibility tests already? (since they contain the old-style inalloca, and verify that when read in it's upgraded to the new-style) Though I don't mind terribly having a separate test, but each file/tool execution does add up & it's nice to keep them to a minimum when test coverage isn't otherwise compromised.


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

https://reviews.llvm.org/D98146