[AIEX][NFC] refactor Load/Store Imm check #460
Conversation
F-Stuckmann
requested review from
abhinay-anubola,
abnikant,
andcarminati,
gbossu,
katerynamuts,
khallouh,
konstantinschwarz,
martien-de-jong,
niwinanto,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| @@ -2469,13 +2470,11 @@ LoadStoreOpcodes AIE2InstructionSelector::getLoadStoreOpcode( | |||
| if (getLoadStoreSize(I) == 512) { | |||
| unsigned RBID = deriveRegBankID(I.getOperand(0).getReg(), MRI, RBI); | |||
| if (RBID == AIE2::AccRegBankID) { | |||
| FitsImmediateRange = checkImmediateRangeSplitting<6, 32, 32>(Offset); | |||
There was a problem hiding this comment.
micronit: we now do the entire case analysis twice, once for the immediate range and once for the instruction selection.
|
|
||
| template <unsigned NumEncodingBits, unsigned Step> | ||
| bool checkImmediateRange(std::optional<APInt> Immediate) { | ||
| unsigned MaxPow2 = NumEncodingBits + llvm::Log2_64(Step); |
There was a problem hiding this comment.
How about passing Log2_64(Step), so that both parameters have the same unit? I guess that computing pow2(StepBits) is cheaper than computing Log2(Step).
There was a problem hiding this comment.
it is not my code and this change would be nicely suited for a proper cleanup. Because we now have multiple switch cases that checkImmediateRanges
F-Stuckmann
force-pushed
the
stuckmann.refactor.mem.imm.check
branch
from
1c4deef to
45c8642
Compare
| @@ -59,6 +59,10 @@ struct AIEBaseInstrInfo : public TargetInstrInfo { | |||
| // of bundles. | |||
| unsigned LoopSetupDistance; | |||
| }; | |||
| virtual bool isOffsetInImmediateRange(unsigned Opcode, unsigned LoadStoreSize, | |||
| std::optional<APInt> Offset) const { | |||
| llvm_unreachable("Target didn't implement OffsetFitImmRange"); | |||
There was a problem hiding this comment.
nit: message out of sync with function name
| @@ -59,6 +59,10 @@ struct AIEBaseInstrInfo : public TargetInstrInfo { | |||
| // of bundles. | |||
| unsigned LoopSetupDistance; | |||
| }; | |||
| virtual bool isOffsetInImmediateRange(unsigned Opcode, unsigned LoadStoreSize, | |||
There was a problem hiding this comment.
I have a feeling that this may be a best guess for these generic opcodes. Perhaps document whether it should return true or false if there's uncertainty.
| case 1024: | ||
| return checkSignedImmediateRangeSplitting<4, 64, 64>(Offset); | ||
| case 2048: | ||
| return checkSignedImmediateRangeSplitting<4, 64, 192>(Offset); |
There was a problem hiding this comment.
nit: I see that this checks 0 and 192, not the intermediate 64 and 128. I guess the implementation assumes that step divides splitoffset. Perhaps static_assert that?
…ion_compare_with_different_bitwith Also canonicalize select and geq with different width in integer types
This is a Non Functional Change to Move Immediate Checks from Instruction Selection to TII.
The global combiner PR (#405 ) needs this refactor, as well as #451 .