This is an archive of the discontinued LLVM Phabricator instance.

OpaquePtr: Turn inalloca into a type attribute
ClosedPublic

Authored by arsenm on Mar 7 2021, 9:25 AM.

Details

Summary

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.

Diff Detail

Event Timeline

arsenm created this revision.Mar 7 2021, 9:25 AM
arsenm requested review of this revision.Mar 7 2021, 9:25 AM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: bbn, wdng. · View Herald Transcript
dblaikie accepted this revision.Mar 7 2021, 10:48 AM

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.

llvm/lib/IR/Attributes.cpp
466–467

Is it? Could you replace this with an assertion instead?

llvm/test/Bitcode/compatibility-3.6.ll
408–410

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)

llvm/test/Bitcode/inalloca-upgrade.test
2–8

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.

This revision is now accepted and ready to land.Mar 7 2021, 10:48 AM
arsenm added inline comments.Mar 7 2021, 3:08 PM
llvm/lib/IR/Attributes.cpp
466–467

It should never be null, but it is in some cases (I think there are some situations still where callsite attributes are missing byval)

llvm/test/Bitcode/compatibility-3.6.ll
408–410

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

llvm/test/Bitcode/inalloca-upgrade.test
2–8

I was following along with byval, which has its own test like this. I also generally hate the all-in-one style of testing features that are really unrelated. It makes it harder to know what actually broke when one fails.

dblaikie added inline comments.Mar 8 2021, 2:37 PM
llvm/lib/IR/Attributes.cpp
466–467

Any chance of clarifying why it is sometimes null today in the comment?

llvm/test/Bitcode/compatibility-3.6.ll
408–410

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.

llvm/test/Bitcode/inalloca-upgrade.test
2–8

A single file with many tests for different features related to, say, bitcode compatibility, seems good to me.

If the failures are hard to read - there are various FileCheck features which can be used to help improve the failure modes (using CHECK-LABEL to isolate one checking area from another, so they don't bleed together/get cause the failure point to drift a long way from the initial problem).

Having every individual test in a separate file may significantly slow down test execution (especially on Windows, where process overhead is higher, if I recall correctly) and I think it reduces the chance of finding and grouping related tests together - leading to a proliferation of subtly overlapping tests rather than a more systematic approach to how a feature is tested. (though that's always challenging - different people think in different groupings, etc, naming is challenging, etc)

arsenm updated this revision to Diff 332159.Mar 21 2021, 8:01 AM
arsenm added inline comments.
llvm/lib/IR/Attributes.cpp
466–467

This turned out to be down to one unit test passing null to the create function, so I've just deleted it

llvm/test/Bitcode/inalloca-upgrade.test
2–8

If you need to debug the failure, having it be as small as possible is helpful. At least for a text test, you can extract a single function and start from there. For binary tests like this, you're out of luck so I think it's more important to minimize these

rnk accepted this revision.Mar 25 2021, 2:33 PM

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.

llvm/test/Bitcode/compatibility-3.6.ll
408–410

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.

I've reverted this (07e46367bae) because it was causing Bindings/Go/go.test to fail on the buoldbots. Example failure at http://lab.llvm.org:8011/#/builders/107/builds/6075.

Please include the "Differential Revision: ..." line in the commit message - it automatically closes the review with the details of the commit (including updating to show what was committed/what changes were made between review and commit) and helps find the review from the commit, etc.