There's only a tablegen testcase for IntImmLeaf and not a CodeGen one
because the relevant rules are rejected for other reasons at the moment.
On AArch64, it's because there's an SDNodeXForm attached to the operand.
On X86, it's because the rule either emits multiple instructions or has
another predicate using PatFrag which cannot easily be supported at the
same time.
Details
Diff Detail
- Build Status
Buildable 11022 Build 11022: arc lint + arc unit
Event Timeline
LGTM with nitpicks.
Also, when you commit, I would do the AArch64 and X86 changes in different commits.
lib/Target/AArch64/AArch64InstructionSelector.cpp | ||
---|---|---|
709 | I didn't get that comment. | |
utils/TableGen/CodeGenDAGPatterns.h | ||
485 | Could we move these in GlobalISelEmitter? | |
495 | Ditto. |
Thanks for the reviews
lib/Target/AArch64/AArch64InstructionSelector.cpp | ||
---|---|---|
709 | This patch causes the rules involving fpimm0 to be imported so we no longer need to handle that case in the C++. It's not harmful to leave it in, but doing so means we can't tell if the test case passes because tablegen works or because the C++ caught it. The comment is intended to explain why we choose not to handle 0.0 in the C++ code even though it would be valid. | |
utils/TableGen/CodeGenDAGPatterns.h | ||
485 | Ok | |
495 | Ok |
lib/Target/AArch64/AArch64InstructionSelector.cpp | ||
---|---|---|
709 | Make sense. |
utils/TableGen/CodeGenDAGPatterns.h | ||
---|---|---|
495 | Looking at this one a bit closer, I'm not sure I should move it. It makes use of other private members (immCodeUsesAPInt and immCodeUsesAPFloat) so I'd have to make those public to be able to move it. I don't think those other members should be public since they describe properties of getImmCode() which is also private. Would it be ok with you to leave this one here? |
utils/TableGen/CodeGenDAGPatterns.h | ||
---|---|---|
495 | Yep that's fine. |
utils/TableGen/CodeGenDAGPatterns.h | ||
---|---|---|
495 | Thanks |
test/TableGen/GlobalISelEmitter.td | ||
---|---|---|
770 | Is there a reason why GIR_CopyFConstantAsFPImm was not implemented in InstructionSelector? |
test/TableGen/GlobalISelEmitter.td | ||
---|---|---|
770 | The most likely explanation is that there were other issues with importing any of the rules that would have made use of it at the time. Do you have a test case that's hitting this? |
test/TableGen/GlobalISelEmitter.td | ||
---|---|---|
770 | Yes, D45994 hits this. I created D45990 to try to implement GIR_CopyFConstantAsFPImm. The main problem is that the AMDGPUGenGlobalISel.inc won't build, because this enum value is not defined. The pattern that this was generated for, we don't need and won't ever use, so it's not a problem if this remains unimplemented, we just need to be able to build AMDGPUGenGlobalISel.inc . |
I didn't get that comment.
Could you elaborate?