[AIE2P] Combine VST.PUSH.CONV #351
Conversation
abhinay-anubola
requested review from
abnikant,
andcarminati,
F-Stuckmann,
gbossu,
katerynamuts,
khallouh,
konstantinschwarz,
martien-de-jong,
niwinanto,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| @@ -3635,15 +3636,27 @@ std::optional<LoadStoreOpcodes> AIE2PInstructionSelector::getCombinedOpcodeCONV( | |||
| (cast<GIntrinsic>(CombOp).getIntrinsicID() != | |||
| Intrinsic::aie2p_v16accfloat_to_v16bf16 && | |||
| cast<GIntrinsic>(CombOp).getIntrinsicID() != | |||
| Intrinsic::aie2p_v32accfloat_to_v32bf16)) | |||
| Intrinsic::aie2p_v32accfloat_to_v32bf16 && | |||
There was a problem hiding this comment.
May I suggest to split this check using an extra early return, then get the intrinsicid in a separate variable and use that for the rest of the function.
| bool AIE2PInstructionSelector::selectVST_FIFO_CONV(MachineInstr &StoreI, | ||
| MachineRegisterInfo &MRI) { | ||
| // First use is IntrinsicID and the second is PtrReg | ||
| Register ConvResult = (std::next(StoreI.uses().begin(), 2))->getReg(); |
There was a problem hiding this comment.
Given that you retrieve PtrIn below as StorI.getOperand(4).getReg(), I think it would be clearer if you would address ConvResult as StoreI.getOperand(5).getReg() here.
There was a problem hiding this comment.
When we use Operand 5 its breaking because fifo.st.flush.3d Operand 5 is IntrinsicId.
Maybe we need to have extra check for instructions, If we want to address as StoreI.getOperand(5).getReg()
There was a problem hiding this comment.
You are saying that somewhere up the call tree two different builtin signatures can be passed to the same function as StoreI. The order of the uses is luckily the same. I would really handle those two signatures in two different functions.
| Register AvailOut = StoreI.getOperand(2).getReg(); | ||
|
|
||
| Register PtrIn = StoreI.getOperand(4).getReg(); | ||
| Register FifoIn = StoreI.getOperand(7).getReg(); |
There was a problem hiding this comment.
Curious: what happens with operand 6?
There was a problem hiding this comment.
Here Operand 5 and 6 are Mantissa and Exponent which are ConvResults. As VST and CONV are combined now, so passing SrcReg of CONV to VST.CONV
| .addImm(AIE2P::sub_bfp16_x) | ||
| .addReg(ExpIn) | ||
| .addImm(AIE2P::sub_bfp16_e); | ||
| } else { |
There was a problem hiding this comment.
It would be nice to have an assert as to what ConvOp is at this point. It looks as if it is implicit from getCombinedOpcodeCONV
| case AIE2P::VST_FLUSH_512_3D: | ||
| return AIE2P::VST_FLUSH_512_CONV_3D; | ||
| } | ||
| return {}; |
There was a problem hiding this comment.
nit: return std::nullopt
Add VST.PUSH.CONV combine in AIE2PInstructionSelector
Modify VST.FLUSH to VST.FLUSH.CONV in AIEPostSelectOptimize
Corresponding MIR tests were added