Page MenuHomePhabricator

[mlir][RFC] Allow to skip operands type specification in ASM form
Needs ReviewPublic

Authored by vinograd47 on Oct 12 2021, 8:31 AM.

Details

Summary

Based on the following discussion:
https://llvm.discourse.group/t/declarative-assembly-format-requirement-for-type-presence/4399

Relax checks in OperationParser - it allows to skip value type specification,
if the value was already defined in the same block.

Diff Detail

Event Timeline

vinograd47 created this revision.Oct 12 2021, 8:31 AM
vinograd47 requested review of this revision.Oct 12 2021, 8:31 AM
mehdi_amini added a comment.EditedOct 12 2021, 10:01 AM

Leaving aside your syntax issue, I don't understand a use-case where it makes sense in MLIR to have the verifyUsedOnlyInTrivialSSACFG restriction applied on the parent regions transitively. Can you elaborate on this?

Leaving aside your syntax issue, I don't understand a use-case where it makes sense in MLIR to have the verifyUsedOnlyInTrivialSSACFG restriction applied on the parent regions transitively. Can you elaborate on this?

I though about the following case:

^bb2:
  // But the parent of parent - not and we still have forware %0 value declaration 
  scf.if (...) { // It's direct parent region satisfies the restrictions  (single block in region)
    %1 = other.op(%0) // Our Operation with UsedOnlyInTrivialSSACFG trait
  }
  ...

^bb1:
  %0 = some.op() -> tensor<1x2x3x4xf32>
  some.br ^bb2
rriddle requested changes to this revision.Oct 13 2021, 1:37 AM

(Marking as general -1 for now)

This revision now requires changes to proceed.Oct 13 2021, 1:37 AM

Leaving aside your syntax issue, I don't understand a use-case where it makes sense in MLIR to have the verifyUsedOnlyInTrivialSSACFG restriction applied on the parent regions transitively. Can you elaborate on this?

I though about the following case:

^bb2:
  // But the parent of parent - not and we still have forware %0 value declaration 
  scf.if (...) { // It's direct parent region satisfies the restrictions  (single block in region)
    %1 = other.op(%0) // Our Operation with UsedOnlyInTrivialSSACFG trait
  }
  ...

^bb1:
  %0 = some.op() -> tensor<1x2x3x4xf32>
  some.br ^bb2

Right, I understand why is it a problem for your syntax. But I don't think this is a valid reason by itself to add such a trait, so I was wondering if this would make sense in another context / for another use-case?

(Marking as general -1 for now)

@rriddle Could you please be a little more specific about this -1? I've tried to preserve original behavior and restrictions as a default state. But just allow users to explicitly switch off this restrictions in their own projects, if needed.

Right, I understand why is it a problem for your syntax. But I don't think this is a valid reason by itself to add such a trait, so I was wondering if this would make sense in another context / for another use-case?

I can implement this in another way - just a flag in ODS (bit strictOperandsTypeCheck = 1?). Will it be OK for you?

(Marking as general -1 for now)

@rriddle Could you please be a little more specific about this -1? I've tried to preserve original behavior and restrictions as a default state. But just allow users to explicitly switch off this restrictions in their own projects, if needed.

Right, I understand why is it a problem for your syntax. But I don't think this is a valid reason by itself to add such a trait, so I was wondering if this would make sense in another context / for another use-case?

I can implement this in another way - just a flag in ODS (bit strictOperandsTypeCheck = 1?). Will it be OK for you?

Mehdi is doing a better job than I, but I'm not yet convinced that this complexity is worth it.

vinograd47 retitled this revision from [mlir][RFC] Relax ODS assembly format rules for operand types presence to [mlir][RFC] Allow to skip operands type specification in ASM form.
vinograd47 edited the summary of this revision. (Show Details)

@rriddle @mehdi_amini

I've redone the change. Now it doesn't touch the ODS generation logic,
instead I've used Custom Directives in assembly format to implement the desired behavior.
The only change in the core part - relaxed check in the OperationParser.

Could you please take a look one more time?

Maybe you missed my last answer on Discourse, can you take another look there?

Maybe you missed my last answer on Discourse, can you take another look there?

@mehdi_amini Are you talking about the following question?

make it such that the type can’t be elided during printing if the producer isn’t in the same block

I assume I've addressed this in the latest change in the custom directive. So this eliding logic is performed on user side, not in the core part.

Maybe you missed my last answer on Discourse, can you take another look there?

@mehdi_amini Are you talking about the following question?

make it such that the type can’t be elided during printing if the producer isn’t in the same block

I assume I've addressed this in the latest change in the custom directive. So this eliding logic is performed on user side, not in the core part.

I don't think that is really what I was saying though: this does not provide a feature to elide the type only if the producer is in the same block. Right now you just poked a hole through the declarative assembly syntax which allows to make it unsafe.

vinograd47 edited the summary of this revision. (Show Details)

Add more checks to OperationParser::resolveSSAUse to handle the misuse of the feature.

I don't think that is really what I was saying though: this does not provide a feature to elide the type only if the producer is in the same block. Right now you just poked a hole through the declarative assembly syntax which allows to make it unsafe.

@mehdi_amini Please take a look one more time. I've added more checks to OperationParser::resolveSSAUse to catch the wrong usage and add extra tests for invalid cases.