Page MenuHomePhabricator

Adjust assertion when casting to an unregistered operation
ClosedPublic

Authored by mehdi_amini on Aug 25 2020, 10:13 PM.

Details

Summary

This assertion does not achieve what it meant to do originally, as it
would fire only when applied to an unregistered operation, which is a
fairly rare circumstance (it needs a dialect or context allowing
unregistered operation in the input in the first place).
Instead we relax it to only fire when it should have matched but didn't
because of the misconfiguration.

Diff Detail

Event Timeline

mehdi_amini created this revision.Aug 25 2020, 10:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2020, 10:13 PM
mehdi_amini requested review of this revision.Aug 25 2020, 10:13 PM

The relationship between the op classes and registration feels under documented and easily error prone to new users. The fact that isa<AffineForOp>(op) will silently fail on an affine.for operation because the affine dialect isn't registered will likely feel wrong to many as an initial reaction. I'm not saying we should allow or encourage the use of the *Op classes when something is unregistered(we shouldn't), but it feels like we could make debugging/understanding a misconfiguration much easier; which if I recall is the original intention of the assert. E.g. as a bare bone hint, could we add a debug log that detects such situations and issues a message? e.g. something like:

LLVM_DEBUG({
  if (op->getName().getStringRef() == ConcreteType::getOperationName())
    dbgs() << "classof on '" << ConcreteType::getOperationName() << "' failed due to the operation not being registered\n";
})

The fact that isa<AffineForOp>(op) will silently fail on an affine.for operation because the affine dialect isn't registered will likely feel wrong to many as an initial reaction

My reasoning is that by default we don't allow to create an unregistered AffineForOp in the IR in the first place: the situation you're describing can only happen if the user use --allow-unregistered-dialects I believe.

But your suggestion seems fine as well for such cases!

Address comment

mehdi_amini retitled this revision from Remove assertion when casting to an unregistered operation to Adjust assertion when casting to an unregistered operation.Aug 25 2020, 11:09 PM
mehdi_amini edited the summary of this revision. (Show Details)
rriddle accepted this revision.Aug 25 2020, 11:17 PM
This revision is now accepted and ready to land.Aug 25 2020, 11:17 PM