[PATCH] D145658: [Xtensa] Initial support of the ALU operations.

[Maciej Czekaj via Phabricator via llvm-commits]

maciejcz added a comment.

The code conforms to Xtensa CALL0 ABI with one clarification needed (about large structures).
Placed some questions about possible dead code inline.



================
Comment at: llvm/lib/Target/Xtensa/XtensaISelLowering.cpp:65
+
+  if (ArgFlags.isByVal()) {
+    Align ByValAlign = ArgFlags.getNonZeroByValAlign();
----------------
Just a nitpick: Xtensa ABI assumes passing larger structures on the stack as opposed to hidden pointer. 
This is not practiced on API boundaries so not a big deal, but perhaps we should document it somewhere in the comments.
The future clang behavior will depend on ABIInfo implementation, which can easily prevent pass by value for structures not fitting in the registers.


================
Comment at: llvm/lib/Target/Xtensa/XtensaISelLowering.cpp:77
+  // Promote i8 and i16
+  if (LocVT == MVT::i8 || LocVT == MVT::i16) {
+    LocVT = MVT::i32;
----------------
i8 and i16 seem to be automatically sign-extended by LLVM. Function:
```
define i8 @foo(i8 %x) {
  ret i8 %x
}

```
does not trigger this case (LocVT == i32).
Is there any other case when this code is used?



================
Comment at: llvm/lib/Target/Xtensa/XtensaISelLowering.cpp:99
+    LocVT = MVT::i32;
+  } else if (ValVT == MVT::f64) {
+    // Allocate int register and shadow next int register.
----------------
f64 arguments are treated as i64 if there is no backing register class. See:  TargetLoweringBase.cpp:1364. 
The code below:
```
define double @foo(double %x) {
        ret double %x
}
```
triggers the former `if` case, i.e. ValVT == i32, isI64 == true. 
Is this condition needed then?


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

https://reviews.llvm.org/D145658