[AIE2P] Fix Accumulator Register Bank assignment for G_STORE #264
Conversation
khallouh
requested review from
abhinay-anubola,
abnikant,
andcarminati,
gbossu,
konstantinschwarz,
martien-de-jong,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| } | ||
| return -1; | ||
| } | ||
|
|
There was a problem hiding this comment.
This looks good, can you add a test ?
There was a problem hiding this comment.
I can't add a test triggering this bug because there is no intrinsic that ouputs an accumulator at index zero. but we can't assume that VReg is always defined at index 0 of DefMI as we don't know what DefMI will be.
( See replaced line: auto *RB = getRegBank(DefMI->getOperand(0).getReg(), MRI, TRI) )
| @@ -766,11 +777,13 @@ AIE2PRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const { | |||
| break; | |||
| MachineInstr *DefMI = MRI.getVRegDef(VReg); | |||
| LLT Type = MRI.getType(VReg); | |||
| auto *RB = getRegBank(DefMI->getOperand(0).getReg(), MRI, TRI); | |||
| auto DefRegBank = getRegBank( | |||
| DefMI->getOperand(getVRegIndexInInstr(*DefMI, VReg)).getReg(), MRI, | |||
There was a problem hiding this comment.
Is DefMI->getOperand(getVRegIndexInInstr(*DefMI, VReg)).getReg() same as VReg?
There was a problem hiding this comment.
DefMI->getOperand(getVRegIndexInInstr(*DefMI, VReg)).getReg() is an operand and VReg is a register. So they cannot be the same.
| return i; | ||
| } | ||
| } | ||
| return -1; |
There was a problem hiding this comment.
What about a std::optional? This -1 looks dangerous regarding alternative uses.
For
G_STORE, we look at the bank of the register in the defining instruction but we assume that this will be at index zero, which has been the case for AIE2P's intrinsics defining accumulators but not true in general.