This is an archive of the discontinued LLVM Phabricator instance.

[Xtensa] Initial support of the ALU operations.
Needs ReviewPublic

Authored by andreisfr on Mar 9 2023, 12:01 AM.

Details

Summary

Initial codegen infrastructure support for simple ALU operations.

Diff Detail

Unit TestsFailed

Event Timeline

andreisfr created this revision.Mar 9 2023, 12:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 12:01 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
andreisfr requested review of this revision.Mar 9 2023, 12:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 12:01 AM
andreisfr updated this revision to Diff 508568.Mar 27 2023, 4:31 AM

Minor code formatting.

andreisfr edited the summary of this revision. (Show Details)Mar 27 2023, 4:09 PM

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.
This should be just RET or RETURN.

llvm/lib/Target/Xtensa/XtensaTargetMachine.h
40

This method should not exist. Subtarget is per-function.

JKRhb added a subscriber: JKRhb.Mar 30 2023, 12:55 PM
barannikov88 added a comment.EditedMar 30 2023, 8:31 PM

I briefly glanced at the other patches and can't review / accept them without good test coverage.

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.
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.

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).
Is there any other case when this code is used?

100

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?

andreisfr updated this revision to Diff 544641.Jul 27 2023, 1:04 AM

Corrected according to comments. Added test with code generation for ALU operations

arsenm added inline comments.Aug 1 2023, 6:01 PM
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

arsenm requested changes to this revision.Aug 24 2023, 4:05 AM
arsenm added inline comments.
llvm/lib/Target/Xtensa/XtensaISelLowering.cpp
196–199

getNode(== f32 ? bitcast : trunc)

211

getStoreSize?

220

getFrameIndexTy (also this should just be the type the pointer already is)

This revision now requires changes to proceed.Aug 24 2023, 4:05 AM
andreisfr updated this revision to Diff 558214.Thu, Dec 7, 12:10 AM

Updated according latest changes in main branch and reviewers comments.