[X86][tablgen] Auto-gen broadcast tables #73654
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This looks to be the right direction to me - please can you update the test failures? |
Yeah. The patch is still in progress. The generated broadcast tables are incorrect and incomplete now, so we can't update the tests directly. @RKSimon |
KanRobert
changed the title
GenericDomain -> SSEPackedInt Found by #73654
Split NFC in #73654 into a seperate commit.
| {X86::VFCMULCPHZrr, X86::VFCMULCPHZrmb, TB_BCAST_SH}, | ||
| {X86::VFMULCPHZ128rr, X86::VFMULCPHZ128rmb, TB_BCAST_SH}, | ||
| {X86::VFMULCPHZ256rr, X86::VFMULCPHZ256rmb, TB_BCAST_SH}, | ||
| {X86::VFMULCPHZrr, X86::VFMULCPHZrmb, TB_BCAST_SH}, |
There was a problem hiding this comment.
The FMULCPH/FCMULCPH fp16 complex instructions are 32-bit broadcasts (2 x fp16) - not sure if there are others?
There was a problem hiding this comment.
Do you mean TB_BCAST_SS should be used for FMULCPH/FCMULCPH or we should not add this entry at all? TB_BCAST_SH is newly added , so I am not clear how you will use it.
There was a problem hiding this comment.
Complex instructions use a pair of FP16 type, so it's 32-bit in fact. Non-complex is 16-bit.
There was a problem hiding this comment.
I think we can safely perform this with a custom entry using TB_BCAST_SS, but it would be OK just to disable them for now (and add a TODO comment).
There was a problem hiding this comment.
Done 56fd306
It's easy so I fixed it directly.
| @@ -508,19 +610,28 @@ void X86FoldTablesEmitter::updateTables(const CodeGenInstruction *RegInst, | |||
| isMemoryOperand(MemOpRec)) { | |||
| switch (I) { | |||
| case 0: | |||
| assert(!IsBroadcast && "BroadcastTable0 needs to be added"); | |||
There was a problem hiding this comment.
What's stopping us adding BroadcastTable0?
There was a problem hiding this comment.
Nothing. We do not add BroadcastTable0 just b/c it's empty now. If there is an entry should be in BroadcastTable0 one day, the assertion will fail and tell us to add the table.
|
Thanks for the review! @RKSimon @phoebewang |
issue: #66360