[PATCH] D91097: [IR] [TableGen] Cleanup pass over the IR TableGen files, part 2

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:166-174
+         !foldl([]<list<WMMA_REGS>>, !if(!size(TypeB), TypeB, [type_a]), t3,
+                type_b, !listconcat(t3,
+           !foldl([]<list<WMMA_REGS>>, TypeC, t4, type_c, !listconcat(t4,
+             !foreach(type_d, !if(!size(TypeD), TypeD, [type_c]),
+                      [WMMA_REGS<geom, "a", type_a>,
+                       WMMA_REGS<geom, "b", type_b>,
+                       WMMA_REGS<geom, "c", type_c>,
----------------
Paul-C-Anagnostopoulos wrote:
> tra wrote:
> > 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.
> > 
> Yes, now that you bring up consistency, I think it would be better to keep !foldl.
> 
> Why do you like them not indented? 
> 
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91097