[PATCH] D99883: [TableGen] Make behavior of list slice suffix consistent across all values

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/TableGen/Record.cpp:618-620
+  if (Elements.size() == 1)
+    return getElement(0);
+
----------------
Paul-C-Anagnostopoulos wrote:
> dblaikie wrote:
> > Paul-C-Anagnostopoulos wrote:
> > > dblaikie wrote:
> > > > Where's the code that already supports the non-literal case? Is there any chance of collapsing these two cases together?
> > > It's in TypedInit::convertInitListSlice(). It's sufficiently different that parameterizing a common subfunction would be more complicated than simply repeating the few common-ish lines of code. 
> > What if convertInitListSlice always returned in the ListInit form, but TGParser.cpp ParseValue checked for length 1 and did the work to turn it into a VarListElementInit or whatnot?
> I assume you're still trying to combine the two convertInitListSlice() functions. But in the loop, the thing that is push_back'ed onto the vector is different in the two functions. And the common case of a single index would build a vector for no reason. I suppose we could build a list of the Inits and then convert to a list of TypeInits, but is that really simpler?
> I assume you're still trying to combine the two convertInitListSlice() functions.

Well, combine the "1 element list becomes an element instead of a list" functionality so it's implemented in one place, yeah. It looks like the "listify this thing" functionality is a bit different and couldn't be unified, but maybe the "1 element list becomes an element instead of a list" part could be unified.

I'd probably be OK with building a vector of 1 element to unify the handling here - unless that proved to be particularly performance prohibitive. 




================
Comment at: llvm/lib/TableGen/Record.cpp:618-620
+  if (Elements.size() == 1)
+    return getElement(0);
+
----------------
Paul-C-Anagnostopoulos wrote:
> dblaikie wrote:
> > Paul-C-Anagnostopoulos wrote:
> > > dblaikie wrote:
> > > > Paul-C-Anagnostopoulos wrote:
> > > > > dblaikie wrote:
> > > > > > Where's the code that already supports the non-literal case? Is there any chance of collapsing these two cases together?
> > > > > It's in TypedInit::convertInitListSlice(). It's sufficiently different that parameterizing a common subfunction would be more complicated than simply repeating the few common-ish lines of code. 
> > > > What if convertInitListSlice always returned in the ListInit form, but TGParser.cpp ParseValue checked for length 1 and did the work to turn it into a VarListElementInit or whatnot?
> > > I assume you're still trying to combine the two convertInitListSlice() functions. But in the loop, the thing that is push_back'ed onto the vector is different in the two functions. And the common case of a single index would build a vector for no reason. I suppose we could build a list of the Inits and then convert to a list of TypeInits, but is that really simpler?
> > > I assume you're still trying to combine the two convertInitListSlice() functions.
> > 
> > Well, combine the "1 element list becomes an element instead of a list" functionality so it's implemented in one place, yeah. It looks like the "listify this thing" functionality is a bit different and couldn't be unified, but maybe the "1 element list becomes an element instead of a list" part could be unified.
> > 
> > I'd probably be OK with building a vector of 1 element to unify the handling here - unless that proved to be particularly performance prohibitive. 
> > 
> > 
> Oh wait, my last sentence makes no sense. Anyway, does taking some of the logic out of the two functions really make it simpler overall? I guess I'm still confused about what you're suggesting.
> I assume you're still trying to combine the two convertInitListSlice() functions.

Well, combine the "1 element list becomes an element instead of a list" functionality so it's implemented in one place, yeah. It looks like the "listify this thing" functionality is a bit different and couldn't be unified, but maybe the "1 element list becomes an element instead of a list" part could be unified.

I'd probably be OK with building a vector of 1 element to unify the handling here - unless that proved to be particularly performance prohibitive. 

> does taking some of the logic out of the two functions really make it simpler overall?

I think so - means updates/changes to the code aren't likely to fix one place and fail to fix the other place, for instance.

But up to you/@lattner - if you find this to be better/easier to maintain this way, that's OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99883