[llvm-mca][RISCV] Fix checking if data valid in createInstrument

[michaelmaitland]

Summary

Fixes createInstrument to return instrument when LMUL data is valid, and
return nullptr when LMUL data is not valid for RISCV target.

[myhsu]

LGTM
Though I'm a little surprised it doesn't print a warning or even error for invalid instruments. Is there any reason behind this?

[myhsu]

Also I'm curious if there is any need to update existing LMUL tests as the instrument didn't even kick in previously? It will be great if you can add a test that checks against invalid LMUL instruments as well.

[michaelmaitland]

Though I'm a little surprised it doesn't print a warning or even error for invalid instruments. Is there any reason behind this?

Debug warning is printed on line 85 of RISCVCustomBehaviour.cpp. We print an actual error and exit from MCA here

[andreadb]

In D149068#4292725, @michaelmaitland wrote:

Though I'm a little surprised it doesn't print a warning or even error for invalid instruments. Is there any reason behind this?

Debug warning is printed on line 85 of RISCVCustomBehaviour.cpp. We print an actual error and exit from MCA here

It would be better if all this custom behaviour logic was properly tested. If I remember it correctly, the plan was to contribute a generic scheduling model as a follow-up to test all of this. Is that still a plan?

[craig.topper]

In D149068#4294263, @andreadb wrote:

In D149068#4292725, @michaelmaitland wrote:

Though I'm a little surprised it doesn't print a warning or even error for invalid instruments. Is there any reason behind this?

Debug warning is printed on line 85 of RISCVCustomBehaviour.cpp. We print an actual error and exit from MCA here

It would be better if all this custom behaviour logic was properly tested. If I remember it correctly, the plan was to contribute a generic scheduling model as a follow-up to test all of this. Is that still a plan?

It's not clear how to define a "generic" scheduling model. An in order CPU and an out of order CPU might have very different implementations for the RISC-V vector extension. Creating "generic" scheduler starts becoming equivalent to architecting a CPU.

We're hoping to upstream a scheduler based on a real hardware implementation soon.

[andreadb]

In D149068#4296291, @craig.topper wrote:

In D149068#4294263, @andreadb wrote:

In D149068#4292725, @michaelmaitland wrote:

Though I'm a little surprised it doesn't print a warning or even error for invalid instruments. Is there any reason behind this?

Debug warning is printed on line 85 of RISCVCustomBehaviour.cpp. We print an actual error and exit from MCA here

It would be better if all this custom behaviour logic was properly tested. If I remember it correctly, the plan was to contribute a generic scheduling model as a follow-up to test all of this. Is that still a plan?

It's not clear how to define a "generic" scheduling model. An in order CPU and an out of order CPU might have very different implementations for the RISC-V vector extension. Creating "generic" scheduler starts becoming equivalent to architecting a CPU.

We're hoping to upstream a scheduler based on a real hardware implementation soon.

Sounds good. Thanks for the info.

llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp

 SharedInstrument
 RISCVInstrumentManager::createInstrument(llvm::StringRef Desc,
                                          llvm::StringRef Data) {
   if (Desc != RISCVLMULInstrument::DESC_NAME) {
     LLVM_DEBUG(dbgs() << "RVCB: Unknown instrumentation Desc: " << Desc
                       << '\n');
     return nullptr;
   }
-  if (RISCVLMULInstrument::isDataValid(Data)) {
+  if (!RISCVLMULInstrument::isDataValid(Data)) {
     LLVM_DEBUG(dbgs() << "RVCB: Bad data for instrument kind " << Desc << ": "
                       << Data << '\n');
     return nullptr;
   }
   return std::make_shared<RISCVLMULInstrument>(Data);
 }
 
 unsigned RISCVInstrumentManager::getSchedClassID(