HomePhabricator

[TableGen] Make behavior of list slice suffix consistent across all values

Authored by Paul-C-Anagnostopoulos on Apr 5 2021, 6:13 AM.

Description

[TableGen] Make behavior of list slice suffix consistent across all values

Differential Revision: https://reviews.llvm.org/D99883

Event Timeline

Hi Paul,

Could you add a test that checks that ? in the input lists are preserved in the extracted slice? (even for 1-element slices).
We have a downstream predicate that checks whether a list has any ?'s in it and after this commit it's returning the wrong value for single-bit extractions where the bit is ?. If I revert this commit it passes. I haven't debugged tablegen itself yet but I expect what's going on is {?, 1}[0] is now ? (not {?}) which is converted to 0 whereas it used to be {?} which was inspected by our predicate that in turn found the ?. (I'm also just noticing that {?} doesn't get printed by -print-records, we've only used it with the predicate around it before).

Hopefully nobody else depends on single-bit extractions coercing to zero :-)

Hmm. I'm not sure why this change would affect bits extractions, since it's only the list slice function that changed. I'll look into it today.

Wow, I managed to add two lines of code with a bug in it, and write a test that didn't uncover the bug. Let me fix it.

@dsanders, could you give me an exact example of what no longer works? I don't think you meant '{?, 1}[0]', because you can't take a list slice of a bits value. I've definitely got a bug, but I'm not sure how it relates to your problem.

You're right the bit slices haven't changed behaviour, the one that has is list slices. Sorry about that, the same test has bit slices nearby and it's trying to test bit/list slices are consistent w.r.t our !iscomplete() predicate.

The relevant bit of the original test is:

def Inputs {                                                                                                                         
  list<bit> LstBitD = [ ?, 0, 1 ];                                                                                                   
} 
def Z7LstBits {
  // !iscomplete(X) evaluates using UnOpInit->isComplete()
  bit A = !iscomplete(Inputs.LstBitA);
  bit B = !iscomplete(Inputs.LstBitB);
  bit C = !iscomplete(Inputs.LstBitC);
  // Test these two explicitly because unset is not convertible to int but an
  // unset bit extracted from a a bits<> is convertible to int.
  // (the original test had a similar test for bits<> too)
  bit D = !iscomplete(Inputs.LstBitD[0]);
  bit E = !iscomplete(Inputs.LstBitD[1]);
}

it's E that's changed behaviour, it used to be 1 but now it's 0.

I've adapted that test to remove the downstream-only predicate:

def Inputs {                                                                                                                         
  list<bit> LstBitD = [ ?, 0, 1 ];                                                                                                   
} 
def Z7LstBits {                                                                                                                      
  list<bit> X1 = Inputs.LstBitD[0];                                                                                                  
  bit X2 = Inputs.LstBitD[0]; // The slice isn't an error but the conversion is                                                      
  bit X3 = X1[0];                                                                                                                    
}

Without this commit:

def Inputs {
  list<bit> LstBitD = [?, 0, 1];
}
def Z7LstBits {
  list<bit> X1 = [?];
  bit X2;
  bit X3 = ?;
}

With this commit:

def Inputs {
  list<bit> LstBitD = [?, 0, 1];
}
def Z7LstBits {
  list<bit> X1 = ?;
  bit X2 = ?;
  bit X3 = X1[0];
}

Looking at the test output without debugging it:

  • X1 looks wrong to me, I think it should be [?] because it's a list type
  • X2 looks correct to me
  • I think X3 failed to evaluate because X1's init isn't a ListInit
  • Our !iscomplete predicate probably has a bug where operators count as incomplete. We do check for isConcrete() though.

@dsanders

Okay, there is a lot going on here.

If you look at the two lines I added to Record.cpp, you'll see that I did an awesome job of it. It always fetches element 0 of the list, regardless of the actual subscript. So the last two fields in Z7LstBits will have element 0, the ?.

Once I fix that, it will still be the case that X1 in your test case will be equal to ?. It is perfectly okay for a list<bit> to have the value ?. There is no automatic conversion from ? to [?] just because the field type is a list.

If you want to fetch a single element from a list and end up with a list, you can always code:

list<int> X = [ some_list[0] ];

Now, regarding your interesting bang operator !iscomplete(). What do you expect it to do if its argument is incomplete when it is first parsed, but ends up complete at the end of the record?

@dsanders

Okay, there is a lot going on here.

If you look at the two lines I added to Record.cpp, you'll see that I did an awesome job of it. It always fetches element 0 of the list, regardless of the actual subscript. So the last two fields in Z7LstBits will have element 0, the ?.

Ah, yep that'll do it. When I was reading that yesterday I looked at it and went, "yep, that's right" too. :-)

Once I fix that, it will still be the case that X1 in your test case will be equal to ?. It is perfectly okay for a list<bit> to have the value ?. There is no automatic conversion from ? to [?] just because the field type is a list.

That makes sense. I was thinking along the lines of not converting [?] to ?. Is the conversion to the type of the field only enforced when an operator or tablegen backend imposes it?

If you want to fetch a single element from a list and end up with a list, you can always code:

list<int> X = [ some_list[0] ];

Now, regarding your interesting bang operator !iscomplete(). What do you expect it to do if its argument is incomplete when it is first parsed, but ends up complete at the end of the record?

It uses isConcrete() to defer evaluation until all the field references have been resolved. At that point it expects all the field references and operators to have been folded away and can inspect whichever Init it ended up with using isComplete().
The end result should be that it only evaluates at the end of the record or when there's no longer any possibility of changing from incomplete to complete. IIRC, the problem we were trying to fix at the time was that we couldn't compare ? and 0 because it would coerce ? to 0 first.

That makes sense. I was thinking along the lines of not converting [?] to ?. Is the conversion to the type of the field only enforced when an operator or tablegen backend imposes it?

It's enforced more or less universally, except that any field can have the uninitialized value ?.

It uses isConcrete() to defer evaluation until all the field references have been resolved. At that point it expects all the field references and operators to have been folded away and can inspect whichever Init it ended up with using isComplete().
The end result should be that it only evaluates at the end of the record or when there's no longer any possibility of changing from incomplete to complete. IIRC, the problem we were trying to fix at the time was that we couldn't compare ? and 0 because it would coerce ? to 0 first.

Ah, okay, that sounds reasonable. Now what is the circumstance under which ? is coerced to 0?


I'm going to Phabricate a fix for this bug tomorrow. I'll include you as a reviewer. If you approve it, then I can push the patch by Monday.

Ah, okay, that sounds reasonable. Now what is the circumstance under which ? is coerced to 0?

From what I remember (it's a few years ago now) it was something to do with bits<> and int but most things I thought it might have been have led to proper errors trying them now.


I'm going to Phabricate a fix for this bug tomorrow. I'll include you as a reviewer. If you approve it, then I can push the patch by Monday.

Ok, I'll keep an eye out for it.