[PATCH] D102852: [RISCV] Fix a crash when lowering split float arguments

Luís Marques via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 21 06:46:02 PDT 2021


luismarques added a comment.

Some thoughts:

1. I agree with you that the psABI doesn't (for now, at least) specify this ABI condition so technically we can do whatever we want. Whether we should still follow the spirit of the ABI or be free to make something completely different I'm not sure about. As you noted, this IR construct might arguably also be outside of what a vector-aware ABI would specify (but clang isn't the only frontend).

2. Besides the psABI not covering vectors explicitly, I think what makes the situation more confusing is that arrays are passed by reference, so the specification of the ABI for non-scalar types ends up being less general than it could be. IMO vectors passed by value are reasonably analogous to the "Floating-point reals" and other structs, the way they are discussed in the spec.

> Since the `Hardware Floating-point Calling Convention` doesn't refer to "aggregates" as much as it refers to "structs" I suppose it follows from your opinion about aggregates that, if the psABI is relevant, we should be looking entirely at the `Integer Calling Convention`? So there shouldn't be any use of floating-point registers in lowering these illegal vectors at all, no matter which ABI is used?

A struct is an aggregate. I think the FP section just happens to use more specific language, as it details the rules for the actual language constructs (structs, reals, etc) that would be impacted by the FP calling convention. But if we decided to follow the spirit of the psABI my overall reading of it is that I would expect small (<= 2×XLEN) vectors passed by value to follow the FP convention, when available.

I'm not opposed to this implementation as "something that works" and that "doesn't have to be compatible with anything" and that "solves our problems now". It's just not the ABI I would expect. It seemed to me like an unnecessary departure from convention, and that's why I didn't find it particularly appealing. But perhaps it doesn't matter? In any case, thanks for the patch and for your analysis. Let's hear from other people -- @kito-cheng @asb and others.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102852/new/

https://reviews.llvm.org/D102852



More information about the llvm-commits mailing list