Page MenuHomePhabricator

Add the TableGen assert statement, step 3
ClosedPublic

Authored by Paul-C-Anagnostopoulos on Thu, Apr 1, 10:58 AM.

Details

Summary

This third step adds support for the 'assert' statement in multiclasses. The statement is now supported at top level and in records, classes, and multiclasses.

@jansvoboda11, once this is pushed, you can add your assertions.

Diff Detail

Event Timeline

Paul-C-Anagnostopoulos requested review of this revision.Thu, Apr 1, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Apr 1, 10:58 AM

Thanks for working on this, Paul!

I left a couple of questions/comments inline.

Also, it would be nice to go through the clang-format and clang-tidy suggestions and applying those that fit into the TableGen coding style.

llvm/lib/TableGen/TGParser.cpp
368

Replacing using AssertionTuple = std::tuple<SMLoc, Init *, Init *> with a regular struct with named members would make this easier to read/understand. Is there any particular reason you chose std::tuple instead?

428

Nit: superfluous newline?

3331

This patch doesn't seem to change the (supposedly) recursive nature of multiclasses. If the comment is stale, I think it would be clearer to move the change into a separate NFC commit explaining the situation.

3445

It looks like the branches used to be lexicographically ordered. If there's no specific benefit to having this be the last element, it might be better to keep it at the top. WDYT?

llvm/lib/TableGen/TGParser.h
41

Shouldn't this be an union {} or llvm::PointerUnion, since it only holds one type at a time? If so, it would be nice to extract into a prep patch.

llvm/lib/TableGen/TGParser.cpp
368

No good reason to use a tuple. The next step is to switch to a struct.

428

I like a line break between two arms of an 'if' when some arms are many lines.

3331

A separate revision just to remove an incorrect comment? Okay.

3445

I will order them alphabetically.

llvm/lib/TableGen/TGParser.h
41

I will investigate this when I convert the assertion tuple to a struct.

jansvoboda11 added inline comments.Thu, Apr 8, 4:39 AM
llvm/lib/TableGen/TGParser.cpp
3331

I think a trivial change like this can be committed without a Phabricator review.

I just pushed this revision and I have no idea how. I meant to push the list slice suffix revision, which I did, and somehow this one went along with it.

I just pushed this revision and I have no idea how. I meant to push the list slice suffix revision, which I did, and somehow this one went along with it.

If something's been committed accidentally/without approval, please revert it in a timely manner - if your git client is in some difficult state that might take time to untangle, it may be best to make a new/separate checkout just so you can revert that patch sooner rather than later.

The reason I haven't reverted this patch is because I don't feel comfortable doing a revert with the tangled up branches I have. Do you think it's safe to revert the commit on my 'assert' branch even though it is not the topmost commit?

The reason I haven't reverted this patch is because I don't feel comfortable doing a revert with the tangled up branches I have. Do you think it's safe to revert the commit on my 'assert' branch even though it is not the topmost commit?

I'm not sure what state your branches are in, so I can't say for sure - hence the suggestion to start a new/separate/clean git checkout and revert it there (you could then throw away that checkout, or you could port the patches from your tangled up checkout instead of fixing it (admittedly fixing it's probably a good experience to figure out what went wrong/how to fix it/etc - but it's not uncommon for folks (myself included) to sometimes just blow things away and start again (porting patches by hand) if it's gotten all messed up))

Ah, so I take it from what you're saying that I can start a new branch and then do a revert of an arbitrary commit on it? I guess there is no reason why not.

Okay, I have reverted this revision.

Ah, so I take it from what you're saying that I can start a new branch and then do a revert of an arbitrary commit on it? I guess there is no reason why not.

Actually I meant a whole new git checkout ( https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git ) - but if your current checkout isn't so messed up that you aren't sure a new branch would be enough, then that's great too!

I addressed some of the comments and the relevant clang clean-ups.

Could I get an LGTM on this revision?

jansvoboda11 requested changes to this revision.Wed, Apr 14, 6:50 AM

Sorry, I didn't realize that by "next step" you were referring to a future patch. I'd like to see my two comments addressed in this (and a prep) patch, instead of in the future.

llvm/lib/TableGen/TGParser.cpp
368

Great, I think it would be best to do so in this patch.

llvm/lib/TableGen/TGParser.h
41

Can you please do it in a separate patch and rebase this one on top?

This revision now requires changes to proceed.Wed, Apr 14, 6:50 AM

Could I do the union as a prep patch, but please delay the change to a struct for a subsequent patch? This code is an extension to the previous steps that use a tuple. It's easier to think about the change to a struct as a separate patch.

In order to use a union in the RecordsEntry struct, I will have to add an enumerated value that specifies which kind of pointer is in the union. Is that worth the bother? Especially since I believe I'll have to write a special destructor.

jansvoboda11 added a comment.EditedThu, Apr 15, 5:16 AM

In order to use a union in the RecordsEntry struct, I will have to add an enumerated value that specifies which kind of pointer is in the union. Is that worth the bother? Especially since I believe I'll have to write a special destructor.

Would introducing a class hierarchy simplify things?

Could I do the union as a prep patch, but please delay the change to a struct for a subsequent patch? This code is an extension to the previous steps that use a tuple. It's easier to think about the change to a struct as a separate patch.

As a reviewer, I'd be much happier approving following patches:

  • patch 1 replaces std::tuple with a well-named members (improvement),
  • patch 2 makes RecordsEntry represent exactly one of Record, ForeachLoop, Assertion (improvement),
  • patch 3 (this one) enables assertions on multiclasses (improvement).

as opposed to approving this patch that (IMO) exacerbates issues in the existing code, with the promise of future patches that will fix things.

Maybe I'm being too pedantic. Feel free to ping other reviewers, I don't want to block this patch if I'm being unreasonable.

I like your improvements, I just want to do the first one last. It's easier for me to get all the assert-related code in and working, then go back and replace the tuple with a struct when all the relevant code is in one branch. I promise to do it and not just forget about it. It's next on my to-do list.

I'm not sure what you mean by introducing a class hierarchy. A RecordsEntry struct already represents just one of the three items that can appear in it. The question here is whether it's worth making it a union if I have to add a discriminator enum and also, I believe, write a custom destructor for it. Right now the discrimination is made simply by checking which item is non-null.

jansvoboda11 accepted this revision.Thu, Apr 15, 9:04 AM

LGTM then.

This revision is now accepted and ready to land.Thu, Apr 15, 9:04 AM

Thanks! I will start a thread on llvm dev about how the union issue.

This revision was landed with ongoing or failed builds.Mon, Apr 19, 6:02 AM
This revision was automatically updated to reflect the committed changes.