Page MenuHomePhabricator

[mlir] ODS attribute subrange lookup
Needs ReviewPublic

Authored by Mogball on Nov 3 2021, 11:48 PM.



Depends on D113128 and D112970.

This is just here for some feedback. I would actually table this for now. Using the subrange getters requires that the op's attributes are formed correctly (all required attributes are present), but there are some traits (e.g. linalg's verifyStructuredOp) that make calls to getODSOperands before Op::verify is called, and these will segfault if a missing required attribute causes the lookup of operand_segment_sizes to fail (i.e. waiting on verification ordering).


Diff Detail

Event Timeline

Mogball created this revision.Nov 3 2021, 11:48 PM
Mogball requested review of this revision.Nov 3 2021, 11:48 PM
Mogball edited the summary of this revision. (Show Details)Nov 4 2021, 12:14 AM
Mogball edited the summary of this revision. (Show Details)
Mogball edited the summary of this revision. (Show Details)
nicolasvasilache added inline comments.

@gysit could you please take a look at the yaml side for this?
Not sure whether we need to pull Stella in too?

gysit added inline comments.Nov 4 2021, 9:57 AM

@Mogball @nicolasvasilache I just tried to understand this and have some questions. The named operations before have no custom verifier. Instead they rely on the LinalgStructuredInterface for verification. It calls verifyStructureOpInterface() in its verifier.

I assume by Op::verfiy you mean some sort of default verifier? Can we control when it is called? Shouldn't it be called before the interface verify anyways?

Mogball added inline comments.Nov 4 2021, 10:53 AM

This is a problem with MLIR's op verification ordering. Traits are verified before Op::verify is called, and there is no (great) way to change this.

The problem arises when traits depend on the op's attributes being well-formed, but the verification of the attributes being well-formed is checked by Op::verify in ODS-generated ops. For example, if a trait queries a variadic operand, the ODS-generated getter calls getAttr("operand_segment_sizes").cast<...>() but there is no guarantee that operand_segment_sizes exists on the op because Op::verify hasn't been called, and it is Op::verify that checks whether operand_segment_sizes exists.

This isn't normally a problem because operand_segment_sizes is added to the op through its ODS-generated builders, but attribute subrange lookups depend on there being n required attributes to the left and m required attributes to the right of the attribute being searched for. So if some of those required attributes are missing, then the subrange lookup could fail to find the attribute when it actually exists.

gysit added inline comments.Nov 4 2021, 11:41 AM

Thanks for the explanation!

Adding a custom verify method on all named operations and calling verifyStructureOpInterface from there may help? It is not a nice solution though.