Richard Biener <[email protected]> writes: >> Not sure whether this is how you understood it, but when I said: >> >> The only valid situations seem to be: >> >> (1) a duplicate of a single zero, where: >> >> npatterns == nelts_per_pattern == encoded_nelts == 1 >> >> and the only encoded value is zero >> >> (2) the combination of: >> >> - nelts_per_pattern == 2 >> - multiple_p (TYPE_VECTOR_SUBPARTS (type), npatterns) >> - the second half of the encoded elements are all zeros >> >> I meant that those conditions seemed to be the ones that your code would >> need to follow in order to fill VLA vectors with zeros. Those conditions >> certainly don't apply to all gimple_build_vector callers. >> >> So to be correct, the original patch would need to add an assert to >> gimple_build_vector that checks the above conditions before using >> constant_lower_bound. Like you say above, without the assert: >> >> { 1, x } nelts_per_pattern == 1, npatterns == 2 >> >> would incorrectly give: >> >> { 1, x, 0, 0, 0, 0, ... } >> >> whereas it should instead be: >> >> { 1, x, 1, x, 1, x, ... } >> >> The current VLS CONSTRUCTOR path is supposedly correct because it >> explicitly initialises every element of the vector (i.e. it does not >> rely on zero padding of the constructor). >> >> My argument was that (1) or (2) above would not come about by chance. >> The caller would have to do something explicit to ensure that (2) is true. >> And if (1) or (2) is true, there is no need to for the tree_vector_builder. >> An array of the leading nonzero elements would be good enough, and would >> be simpler to build. >> >> That's why I thought that a different interface from the tree_vector_builder >> version would be better than adding a complicated assert to the existing >> function. > > My argument is that the current API can cover the new use just fine and > we should avoid introducing new APIs for a single use which could then > well just build a properly constrained CTOR directly. As we've legitimized > the zero-padded VLA typed CTOR variant it is IMHO sensible to have > gimple_build_vector support that case. > > Did I say we have too many APIs already?
Many APIs start out as single use though. The reason for having two APIs in this case is that ctors and vector constants have different representations. The current API matches the vector constant representation and the new one would match the ctor representation. But maybe the two representations are the real issue, and that ctors should have the same representation as constants and be built in the same way as constants (although that might be significantly more awkward for users). >> It's also why... >> >> > The only practical thing that my gimple_build_vector_from_elems function >> > does differently from gimple_build_vector for non-constant vectors is >> > that it does not rely on the vector type having a fixed length: instead, >> > it relies on implicit zero-filling of not-mentioned elements of a >> > CONSTRUCTOR node. Is that behaviour you would be willing to adopt in >> > gimple_build_vector? >> >> ...I don't think that this question really applies (assuming that >> you're asking about the current gimple_build_vector). By design, >> tree_vector_builder implicitly specifies a full vector's worth of >> elements based on a possibly shorter sequence of explicitly elements. >> There are no ambiguities to be resolved. There is nothing that is left >> to gimple_build_vector's interpretation. >> >> If we don't want tree_vector_builder semantics then we should provide >> an interface that doesn't use tree_vector_builder, which is essentially >> my argument above. > > But we have that. Build a CTOR and then an assignment. But I thought the idea of the gimple_build_* interfaces was to avoid unnecessary assignments. I.e. the purpose of the new API would be to fold a ctor to a constant without ever creating a temporary SSA name. Anyway, I suppose I should bow out of this. I'll have wasted a lot of Chris's time by suggesting the path that I did. I do think though that the ctor path in gimple_build_vector should check for the case that it handles (i.e. zero padding), rather than just ignoring the second half of the tree_vector_builder and hoping for the best. Richard
