This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Lower inline asm m with offset to register+imm.
ClosedPublic

Authored by mikhail.ramalho on Mar 16 2023, 10:09 AM.

Details

Summary

As part of D145584, we noticed that llvm was generating suboptimal code
for constraint m when the operand can be be lowered to reg+imm form: it
was being selected as a single register rather than register+imm. This
caused an unnecessary 'addi' to be gen for each m constraint.

This patch changes llvm to select register+imm. This might generate code
that cannot be assembled, but matches gcc's behavior.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 10:09 AM
mikhail.ramalho requested review of this revision.Mar 16 2023, 10:09 AM
asb added a comment.Mar 24 2023, 6:00 AM

I don't feel comfortable approving this because I played a hand in diagnosing the issue and helping Mikhail with the patch. I do think updating our handling of the 'm' constraint to match GCC's is the right thing to do, and this patch achieves it in the simplest way I'm aware of. The logic around asm operand constraints isn't always easy to follow, so it's possible I'm missing alternative ways of achieving the same goal.

kito-cheng accepted this revision.Mar 24 2023, 6:56 AM

LGTM, and m in GCC has some special treatment for vector types, it will forbid offset, but it's kind of because GCC's implementation, so I am not asking to match this behavior.

That's remind me that we should have document those into riscv-c-api-doc or some where...I has a draft but never finished, I guess I should find some time to create a PR to start the discussion.

#include <riscv_vector.h>
int x[100];

int foo(){
    vint32m1_t *a = (vint32m1_t *)&(x[1]); // little bit hack but just used for easier demo the difference
    int y,z;
    asm ("lw %0, %1": "=r"(y) : "m"(*a));
    asm ("lw %0, %1": "=r"(z) : "m"(x[1]));
    return y+z;
}
This revision is now accepted and ready to land.Mar 24 2023, 6:56 AM
craig.topper requested changes to this revision.EditedMar 24 2023, 7:54 PM
This comment has been deleted.
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
185

Is "Expanded" here supposed to be "Expected"?

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2102

The body of an assert is not executed in release builds. SelectAddRegImm will not be called.

This revision now requires changes to proceed.Mar 24 2023, 7:54 PM
  • Fixed error message
  • Fixed function not being called in release mode
mikhail.ramalho marked 2 inline comments as done.Mar 27 2023, 12:33 PM
This revision is now accepted and ready to land.Mar 30 2023, 9:58 PM