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

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.

This looks good from my PoV but make sure all of David's oustanding concerns are addressed.

I think we hoped that the preallocated feature would have replaced inalloca before the opaque type pointer transition happened, but there's no reason to block opaque types on inalloca.



================
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))
----------------
dblaikie wrote:
> arsenm wrote:
> > dblaikie wrote:
> > > 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)
> > Why do these tests have textual IR in them at all then? I don't think the concept of the text IR corresponding to the old bitcode really exists. I would expect to see the current syntax, but I don't actually see why this has a .ll suffix and isn't just a plain text file with check lines
> Seems useful to me to know what we're testing against - without the textual IR, how would we know what the CHECKs are meant to be demonstrating? But I do think the textual IR should be the old/original IR, not updated IR - so we can see the difference.
> 
> Not sure how far from my perspective these tests have already come - maybe it's only a few mistakes that have been repeated, maybe it's more systemic/a more fundamentally different idea about how these tests should be written that other developers have taken when making/maintaining these tests.
I believe the intention of the textual IR is that, if you have a copy of llvm-as 3.6, you can use the text to generate an updated bitcode file. This could be useful if you needed to add more test coverage of an old bitcode record that is changing. I think we shouldn't change the textual IR, just the comments.

Anyone who wishes to regenerate the bitcode already has to revert all the intervening non-comment changes since the last time the bitcode was regenerated.


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

https://reviews.llvm.org/D98146