Initial codegen infrastructure support for simple ALU operations.
Details
Diff Detail
Unit Tests
Time | Test | |
---|---|---|
30 ms | x64 debian > LLVM.CodeGen/Xtensa::alu.ll |
Event Timeline
Half of the includes and forward declarations are unused.
I'm not familiar with the ABI to review this part.
However, manual handling of i1, i8, i16 and f32 looks suspicious, because they are all illegal.
Looks like a dead code to me, but I may be wrong.
llvm/lib/Target/Xtensa/Xtensa.h | ||
---|---|---|
31 | The comment style is always //. | |
llvm/lib/Target/Xtensa/XtensaAsmPrinter.h | ||
38 | Such comments (in other files too) are obvious from the class declaration. | |
llvm/lib/Target/Xtensa/XtensaCallingConv.td | ||
15 | Unused. | |
llvm/lib/Target/Xtensa/XtensaISelDAGToDAG.cpp | ||
78 | This is already printed by -debug-only=isel. | |
82 | Should be dbgs(), errs() is for errors. | |
83 | Node->setNodeId(-1) is missing. It is a subtle bug that is difficult to discover. | |
llvm/lib/Target/Xtensa/XtensaISelLowering.cpp | ||
113 | Missing braces. Around llvm_unreachable above, too. | |
llvm/lib/Target/Xtensa/XtensaISelLowering.h | ||
29 | The FLAG notion is historical (Glue was named Flag one day). Please don't copy it into new targets. | |
llvm/lib/Target/Xtensa/XtensaTargetMachine.h | ||
40 | This method should not exist. Subtarget is per-function. |
I briefly glanced at the other patches and can't review / accept them without good test coverage.
Agreed. It's convenient to add testsuite as a separate patch, but that's inconvenient for reviewers.
We cannot assess whether some code is dead or not. The tests need to be moved along with functionality changes.
The code conforms to Xtensa CALL0 ABI with one clarification needed (about large structures).
Placed some questions about possible dead code inline.
llvm/lib/Target/Xtensa/XtensaISelLowering.cpp | ||
---|---|---|
66 | Just a nitpick: Xtensa ABI assumes passing larger structures on the stack as opposed to hidden pointer. | |
78 | 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). | |
100 | f64 arguments are treated as i64 if there is no backing register class. See: TargetLoweringBase.cpp:1364. define double @foo(double %x) { ret double %x } triggers the former if case, i.e. ValVT == i32, isI64 == true. |
llvm/lib/Target/Xtensa/XtensaCallingConv.td | ||
---|---|---|
20 | Missing space after // | |
llvm/lib/Target/Xtensa/XtensaISelDAGToDAG.cpp | ||
45 | remove dead return | |
77 | Yes, this is redundant printing | |
llvm/lib/Target/Xtensa/XtensaISelLowering.cpp | ||
69–70 | Grammar needs work on this comment | |
71–72 | Why doesn't it work to just leave them unallocated? | |
157 | Use DiagnosticInfoUnsupported and return undefs (or at least report_fatal_error) for unsupported things instead of unreachable | |
181 | use Register |
The comment style is always //.