[PATCH] D40023: [RISCV] Implement ABI lowering

asb added a comment. Thanks Eli, that saved me some time - fixed in https://reviews.llvm.org/rL322514 (clang-x86_64-linux-selfhost-modules-2 is now green). Comment at: lib/CodeGen/TargetInfo.cpp:8913 + } + return getNaturalAlignIndirect(Ty, /*ByVal=*/true); +}

[PATCH] D40023: [RISCV] Implement ABI lowering

efriedma added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:8913 + } + return getNaturalAlignIndirect(Ty, /*ByVal=*/true); +} asb wrote: > efriedma wrote: > > asb wrote: > > > efriedma wrote: > > > > The spec says "Aggregates larger than 2✕XLEN bits

[PATCH] D40023: [RISCV] Implement ABI lowering

efriedma added a comment. Looks like it fails with assertions disabled; no-asserts builds disable value labels, so the IR output looks a little different. Repository: rL LLVM https://reviews.llvm.org/D40023 ___ cfe-commits mailing list

[PATCH] D40023: [RISCV] Implement ABI lowering

asb added a comment. The riscv32-abi and risc64-abi tests are failing (specifically the vararg checks) on the clang-with-thin-lto and modules-slave-2 buildbots: http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/7892

[PATCH] D40023: [RISCV] Implement ABI lowering

This revision was automatically updated to reflect the committed changes. Closed by commit rL322494: [RISCV] Implement RISCV ABI lowering (authored by asb, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D40023?vs=129683=129878#toc

[PATCH] D40023: [RISCV] Implement ABI lowering

asb added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:8913 + } + return getNaturalAlignIndirect(Ty, /*ByVal=*/true); +} efriedma wrote: > asb wrote: > > efriedma wrote: > > > The spec says "Aggregates larger than 2✕XLEN bits are passed by reference

[PATCH] D40023: [RISCV] Implement ABI lowering

efriedma accepted this revision. efriedma added a comment. LGTM Not sure if anyone's mentioned it yet, but there's a C ABI testing tool in clang/utils/ABITest/ which you'll probably want to try at some point. Comment at: lib/CodeGen/TargetInfo.cpp:8913 + } + return

[PATCH] D40023: [RISCV] Implement ABI lowering

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. LGTM, but you should wait for Eli's sign-off, too. https://reviews.llvm.org/D40023 ___ cfe-commits mailing list

[PATCH] D40023: [RISCV] Implement ABI lowering

asb updated this revision to Diff 129683. asb marked 8 inline comments as done. asb added a comment. Rebase after ABIArgInfo signext/zeroext refactoring https://reviews.llvm.org/D41999 / https://reviews.llvm.org/rL322396. We no longer need to modify CGCall.cpp for unsigned 32-bit return values

[PATCH] D40023: [RISCV] Implement ABI lowering

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH asb wrote: > rjmccall wrote: > > asb wrote: > > > rjmccall wrote: > > > > I feel like a better design would

[PATCH] D40023: [RISCV] Implement ABI lowering

asb added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH rjmccall wrote: > asb wrote: > > rjmccall wrote: > > > I feel like a better design would be to record whether to

[PATCH] D40023: [RISCV] Implement ABI lowering

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH asb wrote: > rjmccall wrote: > > I feel like a better design would be to record whether to do a sext or a

[PATCH] D40023: [RISCV] Implement ABI lowering

asb updated this revision to Diff 129607. asb marked 9 inline comments as done. asb added a comment. Herald added a subscriber: niosHD. I've addressed all outstanding comments (a number of response are inline). Additionally: Floats in unions are always passed in GPRs (as clarified here

[PATCH] D40023: [RISCV] Implement ABI lowering

asb added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH rjmccall wrote: > I feel like a better design would be to record whether to do a sext or a zext > in the

[PATCH] D40023: [RISCV] Implement ABI lowering

apazos added a comment. Hi Alex, just a reminder, it looks like Eli's and David's comments have not been addressed yet. https://reviews.llvm.org/D40023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D40023: [RISCV] Implement ABI lowering

majnemer added a comment. In https://reviews.llvm.org/D40023#933466, @asb wrote: > In https://reviews.llvm.org/D40023#933464, @majnemer wrote: > > > So how does something like the following work: > > > > union U { float f; int i; }; > > void f(union U u); > > > > > > The flattening

[PATCH] D40023: [RISCV] Implement ABI lowering

asb added a comment. In https://reviews.llvm.org/D40023#933464, @majnemer wrote: > So how does something like the following work: > > union U { float f; int i; }; > void f(union U u); > > > The flattening described in >

[PATCH] D40023: [RISCV] Implement ABI lowering

majnemer added a comment. So how does something like the following work: union U { float f; int i; }; void f(union U u); The flattening described in https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#hardware-floating-point-calling-convention makes sense for arrays and

[PATCH] D40023: [RISCV] Implement ABI lowering

apazos added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:8872 + else +NeededArgGPRs = 1; + Suggestion to make 1 default value when you declare the var Comment at: lib/CodeGen/TargetInfo.cpp:8938 + + // The size of the actual

[PATCH] D40023: [RISCV] Implement ABI lowering

mgrang added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:8790 +private: + unsigned XLen; + static const int NumArgGPRs = 8; rjmccall wrote: > Is "XLen" a term that anyone working with RISCV would recognize? Because > even if it is, it feels like

[PATCH] D40023: [RISCV] Implement ABI lowering

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH I feel like a better design would be to record whether to do a sext or a zext in the ABIArgInfo. Add

[PATCH] D40023: [RISCV] Implement ABI lowering

mgrang added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:8845 + + // Structures with either a non-trivial destructor or a non-trivial + // copy constructor are always indirectly. Comment sounds incomplete. Comment at:

[PATCH] D40023: [RISCV] Implement ABI lowering

efriedma added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:8913 + } + return getNaturalAlignIndirect(Ty, /*ByVal=*/true); +} The spec says "Aggregates larger than 2✕XLEN bits are passed by reference and are replaced in the argument list with the

[PATCH] D40023: [RISCV] Implement ABI lowering

sdardis added a comment. > Mips is the only other implementer of shouldSignExtUnsignedType but is > unaffected, as unlike classifyArgumentType, classifyReturnType will not > extend i32 values (@sdardis: is this a bug?). I believe this is an oversight that is addressed in the backend. The

[PATCH] D40023: [RISCV] Implement ABI lowering

asb updated this revision to Diff 123020. asb marked an inline comment as done. asb added a comment. Updated to address review comments. I've added some extra test coverage that demonstrates that argument lowering happens the same once registers are exhausted, as well as more coverage around

[PATCH] D40023: [RISCV] Implement ABI lowering

asb added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:8858 + else +NeededArgGPRs = 1; + efriedma wrote: > It looks like the ABI says there's a special rule for varargs here? You're right, I neglected vararg calls. Now addressed and test cases

[PATCH] D40023: [RISCV] Implement ABI lowering

efriedma added reviewers: efriedma, rjmccall. efriedma added a comment. You need more test coverage for the cases where arguments end up on the stack. And some test coverage for varargs calls. Comment at: lib/CodeGen/TargetInfo.cpp:8858 + else +NeededArgGPRs = 1; +

[PATCH] D40023: [RISCV] Implement ABI lowering

asb created this revision. Herald added subscribers: jordy.potman.lists, rbar, arichardson. RISCVABIInfo is implemented in terms of XLen, supporting both RV32 and RV64. Unfortunately we need to count argument registers in the frontend in order to determine when to emit signext and zeroext