Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp | ||
---|---|---|
1418 | nit: double space between \param and Offset | |
1426 | I think comments with var names are usually written like: /*Min=*/-8, /*Max=*/7 | |
1426 | 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? | |
1434 | nit: parentheses are unnecessary | |
3953–3954 | as mentioned above: /*Scale=*/0 | |
llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll | ||
8 | s/structure/structured | |
185 | nit: formatting (and below) | |
247 | nit: formatting |
Thank you for the review @c-rhodes!
Francesco
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp | ||
---|---|---|
1426 | 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 | ||
247 | 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 | ||
---|---|---|
1426 | thanks for clearing it up! | |
llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll | ||
225 | nit: move pred next line | |
247 | 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. |
Thank you for the review @c-rodhes. I have updated the patch on phabricator anyway.
Francesco
llvm/test/CodeGen/AArch64/sve-intrinsics-stN-reg-imm-addr-mode.ll | ||
---|---|---|
247 | I have removed all multiple spaces after the CHECK*:. |
nit: double space between \param and Offset