Add config validity check #376
Conversation
yifeizh2
force-pushed
the
yifei/add_K_constraint
branch
from
8821346 to
ceabcc2
Compare
|
I will further add unit test or local check with benchgc. Currently, we also require the tuner side to reject such invalid config @xurui1995 |
I will add this validating rule on python side |
| return cfgItemCnt == 9; | ||
| } else { | ||
| LLVM_DEBUG(llvm::dbgs() << "The predefined config is invalid\n"); | ||
| assert(validateConfig(config, shape) && "config is invalid"); |
There was a problem hiding this comment.
@zhczhong Replaced the original debug info with assertion to avoid invalid config being replaced with a valid one, which potentially will lead to error in tuning result.
It is not reasonable to add special check on benchgc side and we do not need to maintain two implementations. Throw an exception or error code to told the caller that the config is invalid is a better way, I think. |
Just replaced the original debug message with an assertion. |
Will add that too. And I still prefer adding a validation rule on python side which can help reduce tuning time |
From my perspective, it will be better if function |
| if (validateConfig(config, shape)) | ||
| return true; | ||
| else { | ||
| assert(0 && "config is invalid"); |
There was a problem hiding this comment.
Better to use llvm::dbgs() to give the warning when there's invalid config, assert only throw error in debug mode.
There was a problem hiding this comment.
Better to use llvm::dbgs() to give the warning when there's invalid config,
assertonly throw error in debug mode.
I will update to giving both assert and llvm::dbgs().
| @@ -47,6 +47,12 @@ bool validateConfig(const MatmulConfig &cfg) { | |||
| cfg.NBlock % cfg.innerMostNBlock != 0 || | |||
| cfg.KBlock % cfg.innerMostKBlock != 0) | |||
| return false; | |||
| if (!shape.empty()) { | |||
There was a problem hiding this comment.
If this is the case that generated by heuristic rule, we also need to update the logic there?
BTW, seems it only use the K dim size, no need to pass the entire shape here?
There was a problem hiding this comment.
If this is the case that generated by heuristic rule, we also need to update the logic there? BTW, seems it only use the
Kdim size, no need to pass the entireshapehere?
Heuristic rule will not generate this kind of invalid config. This issue is exposed by tuner side generated config.
The reason for only checking K dim size is because, only the KThreads value will not shrink automatically while M and N would (which is an upstream limitation). I currently passed the entire shape here in case we further need to check shape related constraint for M and N in the future.
There was a problem hiding this comment.
I see. I would suggest we only pass the things that is necessary as you mentioned on M and N, anyway you can keep it as is.
yifeizh2
changed the title
yifeizh2
force-pushed
the
yifei/add_K_constraint
branch
from
9ecf54e to
f2bbd60
Compare
Add matmul config validity check to avoid invalid configs.
Resolves issue #375 and #377.