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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
