Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIE2] Fix missing VST.SRS combine #172

Closed
wants to merge 1 commit into from

Conversation

abhinay-anubola
Copy link
Collaborator

Combined into VST.SRS, if two 256-bit SRS are input to 512-bit STORE through CONCAT

if (getLoadStoreSize(StoreI) != 512 ||
ConcatOp->getOpcode() != AIE2::G_INTRINSIC ||
cast<GIntrinsic>(*ConcatOp).getIntrinsicID() !=
Intrinsic::aie2_concat_I512_I256)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether we would want to normalize this to G_CONCAT_VECTORS. Any way, having common pattern recognizer classes for these common glue patterns (e.g. CONCAT(vsz, vsz) -> vszx2) would really help readability, and be good candidates to have tablegen support for pattern matching.

Copy link
Collaborator

@andcarminati andcarminati Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, if we can solve this in PostLegalizer combiner I think the solution can be simpler. Something like:

X = CONCAT(SRS(A), SRS(B))

To something like:

X = SRS2(CONCAT(A, B))

And let Selector intact.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That indeed seems to be a better way, but I'd rather not split the target-specific handling of intrinsics in one more place, and keep it solely here. I think we all agree the logic for selecting opcodes in this file isn't great, but hopefully we can table-gen-erate most of the logic at some point. Hopefully :)

@abhinay-anubola abhinay-anubola force-pushed the sanubola.combine.VST.SRS branch 2 times, most recently from f0455ca to 89f6712 Compare September 10, 2024 07:55
@andcarminati
Copy link
Collaborator

Hi @abhinay-anubola,

I feel that we need to think in a common approach here. I identified similar case in DivAttributeBroadcasting_aie2_bf16_0:

    %198:vregbank(<16 x s16>) = G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.aie2.v16accfloat.to.v16bf16), %197(<8 x s64>)
    %199:accregbank(<8 x s64>) = G_INTRINSIC intrinsic(@llvm.aie2.ext.512.1024.acc), %196(<16 x s64>), %74(s32)
    %200:vregbank(<16 x s16>) = G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.aie2.v16accfloat.to.v16bf16), %199(<8 x s64>)
    %201:vregbank(<32 x s16>) = G_INTRINSIC intrinsic(@llvm.aie2.concat.bf512.bf256), %198(<16 x s16>), %200(<16 x s16>)
    %224:modregbank(s20) = G_CONSTANT i20 64
    %202:ptrregbank(p0) = G_AIE_POSTINC_STORE %138(<32 x s16>), %38, %224(s20) :: (store (<32 x s16>) into %ir.p_out.0, align 32, !tbaa !4, addrspace 7)
    %225:modregbank(s20) = G_CONSTANT i20 64
    %204:ptrregbank(p0) = G_AIE_POSTINC_STORE %201(<32 x s16>), %202, %225(s20) :: (store (<32 x s16>) into %ir.arrayidx.i.i.i.i.i.i.i.i.i.i.i12, align 32, !tbaa !4, addrspace 7)
    %206:gprregbank(s32) = nsw G_ADD %35, %205
    %211:gprregbank(s32) = G_ICMP intpred(eq), %206(s32), %56
    G_BRCOND %211(s32), %bb.5
    G_BR %bb.6

I think the best here is to have a transparent way to handle those concat/regseq/bitcast among all operations. What do you think?

@abhinay-anubola abhinay-anubola force-pushed the sanubola.combine.VST.SRS branch 2 times, most recently from c5797fc to 5f025ff Compare September 11, 2024 08:05
@abhinay-anubola
Copy link
Collaborator Author

This was solved by: #195

@abhinay-anubola abhinay-anubola deleted the sanubola.combine.VST.SRS branch November 13, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants