[InstrGen] Teach inplaceOperand how to deal with several results #1361
Conversation
|
This was part of #1353, which was abandoned, but could be useful on its own. Thus, here it is! |
|
We could also wait to have a use case. |
|
Up to you. It seems like something we should support and I don't think it's highly likely to be unintentionally broken without also breaking current inplace usage. |
Prior to this commit, it was impossible to describe more than one inplace property. This would limit our ability to set the inplace property on instruction with more than one result. With this patch, it is now possible to define one inplace property list for each output of an instruction. To use this feature simply invoke inplaceOperand method for each output on the declaration of the instruction.
|
Hi @jfix71, I spent some time to see how difficult it would be to add a proper/independent tester for our instr/node generators. I made an additional commit on top of the inplaceOperand fix for a proof-of-context. In a nutshell, this commit makes our builders libraries that we can reuse with testers and it adds expected output to check with. The path forward would be to update the expected outputs (header.h in that case) as we add more features/fix bugs. This is far from being commitable, in particular because it does not enforce the proper dependencies for the check target and uses unix like script for checking the output. I wanted to still share it in case people think it would be a useful feature to have. I believe it could be useful, but on the other hands we also don't modify this code often enough that it is really worth spending a lot of time on this, so I am torned. What do you think? |
|
Cool! Thanks for taking a look at this. I agree it might not make much sense to add the test and its added complexity given that we don't modify the builders very often. I also would imagine in practice it's highly unlikely this would catch a bug that is not caught in other unit tests. That said, it would make debugging much easier in case of such a bug, and more tests are generally a good thing. Overall I'd probably lean against having this, but am also a bit torn. |
|
I've ripped the testing part off this commit. |
Prior to this commit, it was impossible to describe more than one
inplace property. This would limit our ability to set the inplace
property on instruction with more than one result.
With this patch, it is now possible to define one inplace property
list for each output of an instruction. To use this feature
simply invoke inplaceOperand method for each output on the
declaration of the instruction.