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

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/IR/Attributes.cpp:490
     if (Type *Ty = getValueAsType()) {
-      raw_string_ostream OS(Result);
+      // FIXME: This should never be null
       Result += '(';
----------------
dblaikie wrote:
> arsenm wrote:
> > dblaikie wrote:
> > > Is it? Could you replace this with an assertion instead? 
> > It should never be null, but it is in some cases (I think there are some situations still where callsite attributes are missing byval)
> Any chance of clarifying why it is sometimes null today in the comment?
This turned out to be down to one unit test passing null to the create function, so I've just deleted it


================
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)
----------------
dblaikie wrote:
> arsenm wrote:
> > dblaikie wrote:
> > > 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.
> > 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.
> 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)
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


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

https://reviews.llvm.org/D98146