This is an archive of the discontinued LLVM Phabricator instance.

[llvm][CodeGen] Addressing modes for SVE stN.
ClosedPublic

Authored by fpetrogalli on Apr 3 2020, 3:51 PM.

Diff Detail

Event Timeline

fpetrogalli created this revision.Apr 3 2020, 3:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 3:51 PM

Rebase on top of master to trigger the pre-merge checks.

c-rhodes added inline comments.Apr 15 2020, 5:04 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
1417

nit: double space between \param and Offset

1425

I think comments with var names are usually written like: /*Min=*/-8, /*Max=*/7

1425

The parameters here are a bit confusing, N is passed as Root and Base for N and Base? Are both N and Base necessary or could this be refactored?

1433

nit: parentheses are unnecessary

3946

as mentioned above: /*Scale=*/0

llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll
7

s/structure/structured

184

nit: formatting (and below)

246

nit: formatting

fpetrogalli marked 9 inline comments as done.

Thank you for the review @c-rhodes!

Francesco

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
1425

Root and N need to be passed because of the way the ComplexPatterns in tablegen require to define such methods when defining the addressing modes. To make it more explicit what is in input and what is in output in this method, I have changed its signature to return the opcode, the new optimized base and offset as a tuple. The names of the variables should now make it clear what it in input and what is in output.

llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll
246

Not sure what you want me to do here.

@fpetrogalli thanks for patch. Couple of nits that can be addressed before merging, no need to update this patch. LGTM!

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
1425

thanks for clearing it up!

llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll
226

nit: move pred next line

246

I don't think I do either :D

I noticed in the valid upper/lower bound tests you have:

; CHECK:      st3b { z0.b, z1.b, z2.b }, p0, [x0, #-24, mul vl]
; CHECK-NEXT: ret

and others (e.g. st3h_i16):

; CHECK: st3h { z0.h, z1.h, z2.h }, p0, [x0, #6, mul vl]
; CHECK-NEXT: ret

but I know that wasn't what I meant yesterday, feel free to ignore this.

c-rhodes accepted this revision.Apr 16 2020, 2:58 AM
This revision is now accepted and ready to land.Apr 16 2020, 2:58 AM

Thank you for the review @c-rodhes. I have updated the patch on phabricator anyway.

Francesco

fpetrogalli marked 5 inline comments as done.Apr 16 2020, 6:11 AM
fpetrogalli added inline comments.
llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll
246

I have removed all multiple spaces after the CHECK*:.

This revision was automatically updated to reflect the committed changes.
fpetrogalli marked an inline comment as done.