Page MenuHomePhabricator

[TableGen] defm in a loop is not final (bug fix)
Needs ReviewPublic

Authored by hfinkel on May 30 2020, 5:37 AM.

Details

Summary

The TableGen parsing code has a bug: when parsing a defm statement, it tries to do a final resolution even if inside a top-level loop. This doesn't work, however, if things inside the multiclass being resolved need access to the loop-iteration variable. These defm statements need to be delayed until the top-level loop itself is evaluated.

As a quick example, given this:

multiclass D<bit b> {
  def A;

  foreach _ = !if(b, [0], []<int>) in
  def B;
}

foreach ShouldI = 0-1 in
defm MD#ShouldI : D<ShouldI>;

TableGen would provide a parsing error:

foreach-multiclass.td:93:3: error: attempting to loop over '!if(ShouldI, [0], [])', expected a list
  foreach _ = !if(b, [0], []<int>) in
  ^

When the defm resolution is delayed, the evaluation works as expected.

Diff Detail

Event Timeline

hfinkel created this revision.May 30 2020, 5:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2020, 5:37 AM
simon_tatham added inline comments.Jun 15 2020, 5:57 AM
llvm/test/TableGen/foreach-multiclass.td
98

Of course, we've got a proper if statement now! I wonder if it's worth checking that this construction works with that as well as with the foreach dodge?

(It should, because if I remember, if expands to that anyway under the hood. But just in case the implementation changes in future, perhaps.)

hfinkel marked an inline comment as done.Jun 16 2020, 9:39 PM
hfinkel added inline comments.
llvm/test/TableGen/foreach-multiclass.td
98

Yep. In real life, I'm using it with the if statement. Because if expands to this, I put it in the test this way (because this is the foreach-multiclass test, and to make it clearer how the test corresponded to the fix). Would you prefer that I also explicitly add a test with an if statement?

simon_tatham added inline comments.Jun 17 2020, 1:27 AM
llvm/test/TableGen/foreach-multiclass.td
98

That would be my preference, just because the implementation of if by foreach seemed like the kind of shortcut I wouldn't bet on staying true for ever.