Page MenuHomePhabricator

[TableGen] Make behavior of list slice suffix consistent across all values
ClosedPublic

Authored by Paul-C-Anagnostopoulos on Apr 5 2021, 7:03 AM.

Details

Summary

The list slice suffix with a single index is supposed to produce the specified list element. For example:

[0, 1, 2, 3, 4, 5] [3] => 3

This works fine except when the list is a literal (as above) or a defvar, in which case it currently produces a list of the single element:

[0, 1, 2, 3, 4, 5] [3] => [3]

This revision fixes this bug so that everything is consistent. I can find no TableGen files that depend on the inconsistency. I wrote a more comprehensive test.

Diff Detail

Event Timeline

Paul-C-Anagnostopoulos requested review of this revision.Apr 5 2021, 7:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2021, 7:03 AM
dblaikie added inline comments.Apr 5 2021, 2:04 PM
llvm/lib/TableGen/Record.cpp
618–620

Where's the code that already supports the non-literal case? Is there any chance of collapsing these two cases together?

llvm/lib/TableGen/Record.cpp
618–620

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.

dblaikie added inline comments.Apr 6 2021, 4:06 PM
llvm/lib/TableGen/Record.cpp
618–620

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?

llvm/lib/TableGen/Record.cpp
618–620

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?

lattner accepted this revision.Apr 6 2021, 5:09 PM

nice testcase!

This revision is now accepted and ready to land.Apr 6 2021, 5:09 PM
llvm/lib/TableGen/Record.cpp
618–620

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.

dblaikie added inline comments.Apr 6 2021, 5:25 PM
llvm/lib/TableGen/Record.cpp
618–620

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.

618–620

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.

The change makes sense to me. Does this need a documentation update?

An update to the TableGen Programmer's Reference was pushed and is waiting for the build to finish.

Regarding simplifying the convertInitListSlice() functions: I will push this revision as it is, then look at the possibility of simplifying. There is also convertInitializerBitRange(), for which there are three variants.