[AIE2P] Optimize G_PHI regbankselect for fifo/acc #314
Conversation
andcarminati
requested review from
abhinay-anubola,
abnikant,
F-Stuckmann,
gbossu,
katerynamuts,
khallouh,
konstantinschwarz,
martien-de-jong,
niwinanto,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
|
|
||
| # Test 3: The instruction %19:_(p0) = G_PHI %0(p0), %bb.0, %27(p0), %bb.1 | ||
| # should not be mapped to accregbank. The result of the PHI node is used by an instruction | ||
| # that is considered an `accululator-use instruction` but the considered result is just a scalar |
There was a problem hiding this comment.
Oh, did I really write this? Good catch!
andcarminati
force-pushed
the
andreu.fifo.regbank.optimization
branch
from
436679d to
785ff92
Compare
| bb.2: | ||
| PseudoRET implicit $lr | ||
|
|
||
| ... |
There was a problem hiding this comment.
Nit: Maybe add a test where a PHI is used both as a PHI and an ACC. I think we will favour the FIFO mapping at this point.
There was a problem hiding this comment.
Do you mean by both FIFO and ACC? Yes, we should favor the FIFO because we check it first. I hope we will not face this situation, according to the benchmarks I saw, what do you think?
There was a problem hiding this comment.
Why should we favor the FIFO bank?
andcarminati
force-pushed
the
andreu.fifo.regbank.optimization
branch
from
785ff92 to
407735d
Compare
| @@ -966,6 +976,36 @@ AIE2PRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const { | |||
| } | |||
| return AIEBaseRegisterBankInfo::getInstrMapping(MI); | |||
| } | |||
| case TargetOpcode::G_PHI: { | |||
There was a problem hiding this comment.
This is a really special-case to handle. If we map to vector reg bank, we may block optimal SWP scheduling's after. This is also a special case because one single PHI node may contain inputs mapped to fifo, accumulator and vector bank at the same time. The way it is mapped is also different, we handle only the output and inputs are related the inputs.
There was a problem hiding this comment.
Handled in a different way now.
| ... | ||
|
|
||
| # Test 3: The instruction %19:_(p0) = G_PHI %0(p0), %bb.0, %27(p0), %bb.1 | ||
| # should not be mapped to accregbank. The result of the PHI node is used by an instruction |
There was a problem hiding this comment.
Maybe add a similar test for a fifo incompatible type?
| ... | ||
|
|
||
| # Test 4: The instruction %19:_(p0) = G_PHI %0(p0), %bb.0, %27(p0), %bb.1 | ||
| # is also used as PHI input. |
There was a problem hiding this comment.
Nit: do you mean? %23:_(<32 x s32>) = G_PHI %66(<32 x s32>), %bb.0, %28(<32 x s32>), %bb.1
andcarminati
force-pushed
the
andreu.fifo.regbank.optimization
branch
from
407735d to
64294b7
Compare
For G_IMPLICIT_DEF. Looking through PHI nodes.
andcarminati
force-pushed
the
andreu.fifo.regbank.optimization
branch
from
64294b7 to
6a5da0c
Compare
| if (DefReg.isPhysical()) | ||
| continue; | ||
| if (isUseFifoInsn(MRI, TRI, DefReg, Depth + 1)) | ||
| return true; |
There was a problem hiding this comment.
This is great that we now look into the real use. Can you add a test for this with bitcasts and copies? I don't think we have that for the fifo bank.
| continue; | ||
| if (isUseAccInsn(MRI, TRI, DefReg, Depth + 1)) | ||
| return true; | ||
| } else if (MRI.getType(RegOp).getSizeInBits() == 2048 || |
There was a problem hiding this comment.
This makes sense for 2048-bit vectors. Does it address a specific case that we did not handle or is this just to return earlier?
|
I left a couple of comments, where a couple of more tests would be nice. Otherwise, this looks good to me. |
Observation: this PR should include more tests after merging more fifo intrinsics.