Page MenuHomePhabricator

[TableGen] Fix anonymous record self-reference in foreach and multiclass
ClosedPublic

Authored by jyyou.tw on Jan 24 2021, 2:17 AM.

Details

Summary

If we instantiate self-referenced anonymous records in foreach and
multiclass, the NAME value will point to incorrect record.
It's because anonymous name is resolved too early.

This patch adds AnonymousNameInit to represent an anonymous record
name. When instantiating an anonymous record, it will update the
referred name.

Diff Detail

Event Timeline

jyyou.tw created this revision.Jan 24 2021, 2:17 AM
jyyou.tw requested review of this revision.Jan 24 2021, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2021, 2:17 AM

Note that the current behavior is specified in the documentation:

"Every class has an implicit template argument named NAME (uppercase), which is bound to the name of the Def or Defm inheriting the class. The value of NAME is undefined if the class is inherited by an anonymous record."

I can't think of a reason why the current behavior is sacred, but the documentation needs to be updated. I'm happy to do that once this revision is pushed. The description of multiclass also needs work as far as NAME is concerned.

I presume this revision passes all the TableGen, mlir, and clang tests.

llvm/lib/TableGen/TGParser.cpp
453

This makes another pass over all the record fields just to resolve references to the anonymous name. Is there a way to enhance Record::resolveReferences to do this in the usual resolution pass (line 457).

lattner accepted this revision.Jan 24 2021, 2:51 PM

Looks structurally good to me, but I'm not a domain expert here.

This revision is now accepted and ready to land.Jan 24 2021, 2:51 PM
jyyou.tw updated this revision to Diff 319498.Jan 27 2021, 1:48 AM

Another try

jyyou.tw marked an inline comment as done.Jan 27 2021, 2:02 AM

Looks good to me.

About pre-merge check failure, I reran OpenMP tests on local machine but some tests still failed, with or without the change.
It seems that failure is unrelated to the patch. I tried to restart check but had no permission.
What can I do in this situation?

What do you mean that you tried to restart check?

What do you mean that you tried to restart check?

I clicked the "Restart Build" link in this page https://reviews.llvm.org/harbormaster/build/119083/

I know nothing about restarting builds.

Have you pushed this revision yet? I don't understand what a pre-merge check is.

Perhaps @dblaikie can help us.

No, I don't have commit access.

Ah, then I don't think that build matters.

Perhaps David will commit it for you. Otherwise I will learn how to do it.

Ah, then I don't think that build matters.

Perhaps David will commit it for you. Otherwise I will learn how to do it.

Generally if you have commit access and have approved a patch & the patch author mentions they don't have commit access - you can go ahead and commit it for them.

Basically arc patch DXXXX will pull their patch down, and then you can git commit --amend --author=... to set the author information to the original author (I usually end up having to search for the user on the LLVM mailing list to find their real name and email address (so the author can be "Firstname Lastname <user@domain.com>") rather than their phabricator username if possible) and commit/push that like any other patch. I think some of the details are here: https://llvm.org/docs/DeveloperPolicy.html#commit-messages

Okay, I will attempt to push this patch on Friday.

@dblaikie, I don't use Arcanist. Can I just download the raw diff via Phabricator and then apply it with 'git apply'?

@dblaikie, I don't use Arcanist. Can I just download the raw diff via Phabricator and then apply it with 'git apply'?

Sure, whatever works for you :) Guess that way you might have to also manually copy/paste the commit message and title, etc.

I will push this revision on Tuesday.

I will push this revision on Tuesday.

Ideally, if you include the "Differential Revision: " line in the commit, PHabricator will automatically detect the commit closes the revision, and update the revision to mention the closing commit, change the status, etc. (don't feel like you have to post a manual "I closed this" comment or anything, if the automatic thing has done the work - unless there's something you'd like to add)

If that doesn't happen for any reason ( such as in this case) please include the hash of the closing commit in a message and set the status of the revision to "closed".

I will push this revision on Tuesday.

Ideally, if you include the "Differential Revision: " line in the commit, PHabricator will automatically detect the commit closes the revision, and update the revision to mention the closing commit, change the status, etc. (don't feel like you have to post a manual "I closed this" comment or anything, if the automatic thing has done the work - unless there's something you'd like to add)

If that doesn't happen for any reason ( such as in this case) please include the hash of the closing commit in a message and set the status of the revision to "closed".

Oh, sorry - I misread. I thought it said that it had been puished on Tuesday. - ignore all that. i'm sure it'll work out fine :)

I pushed @jyyou.tw 's other revision on Friday and apparently it went swimmingly.

This revision was landed with ongoing or failed builds.Feb 1 2021, 8:01 AM
This revision was automatically updated to reflect the committed changes.