Page MenuHomePhabricator

[TableGen] Fix a bug that MCSchedClassDesc is interfered between different SchedModel
ClosedPublic

Authored by steven.zhang on Sep 24 2019, 2:07 AM.

Details

Summary

Assume that, ModelA has scheduling resource for InstA and ModelB has scheduling resource for InstB. This is what the llvm::MCSchedClassDesc looks like:

llvm::MCSchedClassDesc ModelASchedClasses[] = {
...
InstA, 0, ...
InstB, -1,...
};

llvm::MCSchedClassDesc ModelBSchedClasses[] = {
...
InstA, -1,...
InstB, 0,...
};

The -1 means invalid num of macro ops, while it is valid if it is >=0. This is what we look like now:

llvm::MCSchedClassDesc ModelASchedClasses[] = {
...
InstA, 0, ...
InstB, 0,...
};

llvm::MCSchedClassDesc ModelBSchedClasses[] = {
...
InstA, 0,...
InstB, 0,...
};

And compiler hit the assertion here because the SCDesc is valid now for both InstA and InstB.

if (SCDesc->isValid() && !DefMI->getOperand(DefOperIdx).isImplicit()
      && !DefMI->getDesc().OpInfo[DefOperIdx].isOptionalDef()
      && SchedModel.isComplete()) {
    errs() << "DefIdx " << DefIdx << " exceeds machine model writes for "
           << *DefMI << " (Try with MCSchedModel.CompleteModel set to false)";
    llvm_unreachable("incomplete machine model");
  }

Diff Detail

Event Timeline

steven.zhang created this revision.Sep 24 2019, 2:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 2:07 AM
Herald added a subscriber: zzheng. · View Herald Transcript
jsji added inline comments.Oct 2 2019, 8:22 AM
llvm/test/CodeGen/ARM/ParallelDSP/multi-use-loads.ll
23 ↗(On Diff #221485)

Do these changes in scheduling means ParallelDSP SchedModel has some interference with others?
If so, maybe we need another patch to fix that.

samparker added inline comments.Oct 2 2019, 8:39 AM
llvm/test/CodeGen/ARM/ParallelDSP/multi-use-loads.ll
23 ↗(On Diff #221485)

I think if this patch was rebased, this change would go away. This brought to our attention that some instructions weren't modelled, but I hope this is fixed now.

jsji added inline comments.Oct 2 2019, 10:25 AM
llvm/test/CodeGen/ARM/ParallelDSP/multi-use-loads.ll
23 ↗(On Diff #221485)

Good to know that! Thanks @samparker !

steven.zhang marked an inline comment as done.

rebase the patch.

I have rebased the patch.

llvm/test/CodeGen/ARM/ParallelDSP/multi-use-loads.ll
23 ↗(On Diff #221485)

Good catch, and yes, it has been fixed by https://reviews.llvm.org/rL373163

jsji accepted this revision as: jsji.Oct 8 2019, 9:12 PM

LGTM. It would be great if you can figure out why we still have difference in ARM tests, but it shouldn't block this.

llvm/test/CodeGen/ARM/ParallelDSP/unroll-n-jam-smlad.ll
48

Why do we still have this difference? Shouldn't it be fixed as well?

This revision is now accepted and ready to land.Oct 8 2019, 9:12 PM
This revision was automatically updated to reflect the committed changes.
andreadb added a comment.EditedOct 11 2019, 2:46 AM

I am not convinced that this patch is correct. Isn’t the problem that your model was wrongly marked as complete?

I am not convinced that this patch is correct. Isn’t the problem that your model was wrongly marked as complete?

Nevermind. If the issue was related to the model being marked as complete, then the tablegen backend would have complained earlier on when building the scheduling classes.

I am not convinced that this patch is correct. Isn’t the problem that your model was wrongly marked as complete?

Even we marked the model complete wrongly, the table-gen should generate the data or diagnose correctly. And yes, the table-gen will emit diagnose messages, but still emit the SCDesc as valid, which messy the model.