This is an archive of the discontinued LLVM Phabricator instance.

[IR] [TableGen] Cleanup pass over the IR TableGen files, part 2
ClosedPublic

Authored by Paul-C-Anagnostopoulos on Nov 9 2020, 12:25 PM.

Details

Summary

This is the final patch for the IR TableGen files cleanup.

It deals with the intrinsics for NVVM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2020, 12:25 PM
Paul-C-Anagnostopoulos requested review of this revision.Nov 9 2020, 12:25 PM
tra added a reviewer: tra.Nov 9 2020, 1:03 PM
tra accepted this revision.Nov 9 2020, 1:12 PM

LGTM overall, modulo one cosmetic nit.

llvm/include/llvm/IR/IntrinsicsNVVM.td
173–181

I'd keep the original version, including the formatting -- I think it's substantially more readable that way. Understanding the uniformly nested set of foldl() is mentally easier to grok than the mix of foldl and foreach with args wrapped differently at each level.

IMO, it's one of the cases where nominally better code is not necessarily an improvement overall.

This revision is now accepted and ready to land.Nov 9 2020, 1:12 PM
llvm/include/llvm/IR/IntrinsicsNVVM.td
173–181

Yes, now that you bring up consistency, I think it would be better to keep !foldl.

Why do you like them not indented?

tra added inline comments.Nov 9 2020, 1:54 PM
llvm/include/llvm/IR/IntrinsicsNVVM.td
173–181

When nested foldl's indented identically, it's easier to see that all of them do roughly the same thing.

Considering that each next foldl is an argument for !listconcat, we'd need to indent way too far to the right to make it useful. Or we'd need to wrap foldl arguments so that each is on its own line. That would make it hard to see the whole class at once.
Indenting just a little makes it hard to tell where exactly the next indentation level belongs, so that's not very goo either.
Not indenting, is not perfect, but I find it the least confusing of the options I tried.

Restored the two !foldl operators discussed above.