This is an archive of the discontinued LLVM Phabricator instance.

[lanai] Add Lanai backend.
ClosedPublic

Authored by jpienaar on Feb 8 2016, 5:04 PM.

Details

Summary

Add the Lanai backend to lib/Target.

General Lanai backend discussion on llvm-dev thread "[RFC] Lanai backend" (http://lists.llvm.org/pipermail/llvm-dev/2016-February/095118.html).

Diff Detail

Repository
rL LLVM

Event Timeline

jpienaar updated this revision to Diff 47274.Feb 8 2016, 5:04 PM
jpienaar retitled this revision from to [lanai] Add Lanai backend..
jpienaar updated this object.
jpienaar updated this revision to Diff 47275.Feb 8 2016, 5:18 PM

Add codegen and MC tests.

MatzeB removed a subscriber: MatzeB.Feb 8 2016, 7:19 PM
jpienaar updated this revision to Diff 47338.Feb 9 2016, 10:35 AM

Really add test. Re-uploaded old diff when attempting to add tests.

jpienaar updated this revision to Diff 47565.Feb 10 2016, 4:54 PM

Use LanaiMCExpr as suggested in http://reviews.llvm.org/D17008.

Update Setflag ALU combiner and comments.

Hi,

Sorry it took so long to take a look here.

This seems like a fairly straightforward backend commit, and most of the code looks fine. My comments are generally trivia.

I think the biggest issue is that the CodeGen test coverage is fairly weak. Missing facets I saw:

  • Call/return handling.
  • Frame lowering.
  • Stack handling.
  • The custom passes you've added.
  • Relocations and ELF output (including large & small memory models).
  • Subtarget features, probably.

Cheers.

Tim.

lib/Target/Lanai/LanaiAluCode.h
45 ↗(On Diff #47565)

Why the trailing underscore?

lib/Target/Lanai/LanaiAsmPrinter.cpp
162–165 ↗(On Diff #47565)

Essentially dead code here. Just add the switch if and when it's actually needed.

lib/Target/Lanai/LanaiCondCode.h
36 ↗(On Diff #47565)

Name mismatch.

41 ↗(On Diff #47565)

This second version seems to be only used in an assertion, and even that only checks the length of what's returned. I'm not really convinced it's worth the hassle.

100 ↗(On Diff #47565)

I think this name is misleading given that it uses EndsWith.

lib/Target/Lanai/LanaiISelLowering.cpp
996 ↗(On Diff #47565)

A basic array is simpler for this kind of use.

lib/Target/Lanai/LanaiInstrInfo.cpp
329–330 ↗(On Diff #47565)

I think the RCA needs more documentation, and this seems to be its densest point. Here it looks like you're using it as a manual return address, but if so what's the point of the CALL instruction? And why is there only code to push it but no corresponding pop?

Actually, this seems to be about the only code that refers to it at all, as far as I've found (even via getRARegister()).

lib/Target/Lanai/LanaiInstrInfo.td
749 ↗(On Diff #47565)

Similarly, why does CALL use RCA?

lib/Target/Lanai/LanaiTargetMachine.cpp
34 ↗(On Diff #47565)

(Not at all seriously). Why do this to us???!?

lib/Target/Lanai/MCTargetDesc/LanaiMCExpr.cpp
32 ↗(On Diff #47565)

Is this intentionally mixed & upper case? Yes is an entirely acceptable answer, I just thought it might be an overzealous search/replace (having only just noticed that I did that myself a couple of years ago).

t.p.northover added inline comments.Feb 29 2016, 7:38 AM
lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp
682 ↗(On Diff #47565)

This doesn't really look like a switch kind of control flow.

712–713 ↗(On Diff #47565)

Similarly here, I think we'd mostly use an early exit to reduce indentation.

if (Lexer.getKind() != AsmToken::Identifier)
  return 0;
1002 ↗(On Diff #47565)

nullptr?

1153 ↗(On Diff #47565)

Is the assembler intentionally case-sensitive?

lib/Target/Lanai/InstPrinter/LanaiInstPrinter.cpp
81 ↗(On Diff #47565)

I'd suggest factoring out the common code between the load/store cases, they're almost identical.

lib/Target/Lanai/Lanai.td
32 ↗(On Diff #47565)

LLVM naming convention for variables is CamelCase.

chandlerc edited edge metadata.Feb 29 2016, 7:43 AM

Hi,

Sorry it took so long to take a look here.

This seems like a fairly straightforward backend commit, and most of the code looks fine. My comments are generally trivia.

I think the biggest issue is that the CodeGen test coverage is fairly weak.

I had thought that Jacques had indicated that most of the tests were in a separate patch just for size reasons, but I can't now find that patch....

Hi,

Sorry it took so long to take a look here.

This seems like a fairly straightforward backend commit, and most of the code looks fine. My comments are generally trivia.

I think the biggest issue is that the CodeGen test coverage is fairly weak.

I had thought that Jacques had indicated that most of the tests were in a separate patch just for size reasons, but I can't now find that patch....

NM, the tests got merged in, and we just need more of them.

jpienaar updated this revision to Diff 49684.Mar 2 2016, 4:03 PM
jpienaar edited edge metadata.
jpienaar marked 12 inline comments as done.

Addressed comments from review, modified DebugInfo test to use llc, modified ELFYAML which got moved, and added build files.

jpienaar marked 2 inline comments as done.Mar 2 2016, 4:07 PM

Hi,

Thanks for the comments and suggestions. We can definitely add more tests.

Jacques

lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp
1153 ↗(On Diff #47565)

Yes. That is a bit strict but that is the preference.

lib/Target/Lanai/LanaiAluCode.h
45 ↗(On Diff #47565)

To distinguish it from actual ALU codes. I'll make these separate members outside of the enum instead.

lib/Target/Lanai/LanaiCondCode.h
41 ↗(On Diff #47565)

Fair point, I agree with you and no one has asked for this legacy form so I'll just remove this.

100 ↗(On Diff #47565)

Renamed to suffixToLanaiCondCode.

lib/Target/Lanai/LanaiInstrInfo.cpp
329–330 ↗(On Diff #47565)

Indeed. This is in an intermediate state of sorts: originally there was one version that always returned via RCA, now the return addressed is always pushed/popped from the stack. In future we would want to use RCA where possible. I updated the comment. Let me know if this makes sense.

lib/Target/Lanai/LanaiInstrInfo.td
750 ↗(On Diff #49684)

Seems that might have been a left-over from the change in calling convention. Removed.

lib/Target/Lanai/LanaiTargetMachine.cpp
34 ↗(On Diff #47565)

:-)

lib/Target/Lanai/MCTargetDesc/LanaiMCExpr.cpp
32 ↗(On Diff #47565)

Yes it was intentional but only as we were following Hexagon, MIPS and Sparc here. I don't know if there is a specific convention here that should be following instead.

jpienaar updated this revision to Diff 49787.Mar 3 2016, 5:08 PM
jpienaar marked 2 inline comments as done.

Added test for custom passes, code models, removed subtarget features (always true), and some clean-up.

Thanks for updating the patch. I've not quite got through the new one yet, but had one suggestion for now. Feel free to ignore and batch it up with the rest of my second pass if you want (or object).

Cheers.

Tim.

lib/Target/Lanai/LanaiInstrInfo.cpp
330–331 ↗(On Diff #49787)

OK, l see what's going on now. I think this pass is the germ of a LanaiExpandPseudoInstructions.cpp pass (that happens to only handle CALL for now, but arches tend to accumulate pseudo-instructions for a pastime).

I'd suggest dropping all encoding and assembly information from CALL so that it's purely a pseudo-instruction, and in this pass replacing the CALL with a real BT instruction. This has the advantage (among others) that the MachineInstr representation is always unambiguous: at the moment, if you see a CALL it means something different before this pass to afterwards.

I'd also rework it in its own expansion pass for future use (most targets have an equivalent).

It's also be worth commenting explicitly on the fact that Lanai has a dedicated return instruction (which is essentially a "pop %pc") but no corresponding CALL.

t.p.northover added inline comments.Mar 10 2016, 8:00 PM
lib/Target/Lanai/LanaiTargetMachine.cpp
114 ↗(On Diff #49787)

Oh, and I've just noticed this definitely should be happening after all scheduling (and anything else that might fiddle around) since it hard-codes RCA == PC + 16.

In fact, I'm almost tempted to say you want to do it in MCInstLowering because of that. It's probably where I'd put it myself for my own peace of mind, but I'll leave the decision to you.

jpienaar updated this revision to Diff 50575.Mar 14 2016, 3:51 AM
jpienaar added a reviewer: t.p.northover.
jpienaar marked 2 inline comments as done.
jpienaar removed a subscriber: t.p.northover.

Lower call in MCInstLowering and remove LanaiSaveReturnAddressPass.

Thanks for the suggestion, doing the call lowering via MCInstLowering is better.

Jacques

t.p.northover edited edge metadata.Mar 14 2016, 4:02 PM

Hi again,

Thanks for doing the updates. I think this is looking pretty good now, hopefully even the last round of comments. Mostly trivia, with just one substantial issue over shifts.

Cheers.

Tim.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
496 ↗(On Diff #50575)

The changes to this file aren't intentional, are they?

lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp
368 ↗(On Diff #50575)

nullptr?

lib/Target/Lanai/LanaiInstrInfo.td
429–431 ↗(On Diff #50575)

I think the shifts are a bit messy (especially the immZExt6 and immNegZext6 Operands which don't seem to agree between any 2 places they're mentioned).

The comments in the formats file say that imm16 is sign extended, and only values between -31 and 31 are valid.

What I'd suggest is unifying SHL_I/SRL_I and SHA_I/SRA_I. Their joint operand (called shift_imm, say, because it's not really a 2s-complement imm6 in any sense) then just needs to check "Imm >= -31 && Imm <= 31". You might want to check validity in the decoder, but other than that weird helpers ought to be unnecessary.

This range works for CodeGen too, based on the shl node (negative shifts get weird, but they're UB anyway).

Finally, you'll need separate "def : Pat" patterns to handle srl/sra DAGs.

728–729 ↗(On Diff #50575)

Even the asm can probably be empty here: if these instructions ever get printed something has gone pretty wrong and you'd probably prefer an error.

lib/Target/Lanai/MCTargetDesc/LanaiAsmBackend.cpp
164 ↗(On Diff #50575)

So, what do you really support? This seems like it might be an ad-hoc if(!TheTriple.isOSBinFormatELF()), if the BSDs are OK.

lib/Target/Lanai/MCTargetDesc/LanaiMCTargetDesc.cpp
70 ↗(On Diff #50575)

Similarly isOSBinFormatELF here.

jpienaar updated this revision to Diff 50731.Mar 15 2016, 7:36 AM
jpienaar edited edge metadata.
jpienaar marked 6 inline comments as done.

Changed shift instructions to use shiftImm instead of imm6 and negImm6 variants.

Hey,

Thanks for the thorough review and good suggestions.

Jacques

lib/Target/Lanai/LanaiInstrInfo.td
429–431 ↗(On Diff #50575)

Ha, yes I was just talking about changing this, this past week. But I wanted to avoid too much changes while you were reviewing. Updated.

t.p.northover added inline comments.Mar 15 2016, 10:29 AM
lib/Target/Lanai/InstPrinter/LanaiInstPrinter.cpp
230–231 ↗(On Diff #50731)

I'd be inclined to put the complexity into the decoder instead (i.e. require that the MCInst's immediate is already sign extended), since you probably want to diagnose an attempt to disassemble a dodgy shift there anyway and I don't think symbolic operands are permitted so the default formatHex ought to work fine.

Hi,

Some inline comments here about TargetMachine/Subtarget etc.

In general, btw, if you're planning on using subtargets at all and subtarget features you're going to want to look at ports like AArch64 and X86 for guidance. You're saving some variables off to the side that you don't really need and probably won't ever need. :)

I've pointed out some things inline. I'll take another look after these cleanups :)

Thanks!

-eric

lib/Target/Lanai/LanaiDelaySlotFiller.cpp
38 ↗(On Diff #50731)

This appears to be unused past getting TII.

48 ↗(On Diff #50731)

MF is the standard variable name for MachineFunctions.

lib/Target/Lanai/LanaiFrameLowering.h
31 ↗(On Diff #50731)

Make this a reference?

lib/Target/Lanai/LanaiISelDAGToDAG.cpp
50–51 ↗(On Diff #50731)

Unused.

53–55 ↗(On Diff #50731)

Unused? (In addition the DAG also has a copy of the current Subtarget based on function attributes as well).

lib/Target/Lanai/LanaiISelLowering.h
63 ↗(On Diff #50731)

Unused.

105 ↗(On Diff #50731)

Unused it appears? And you can simplify constructors as well if you don't need it.

lib/Target/Lanai/LanaiSubtarget.h
63 ↗(On Diff #50731)

In general: What's with the underscores? :) Please follow the coding convention here (and everywhere).

71 ↗(On Diff #50731)

Needed?

jpienaar updated this revision to Diff 50804.Mar 16 2016, 2:15 AM
jpienaar marked 9 inline comments as done.

Sign extend shift immediates in the decoder, remove unused variables and correct variables not named according to naming convention.

echristo accepted this revision.Mar 16 2016, 11:32 PM
echristo added a reviewer: echristo.

Some pretty particularly nitpicky comments. That said, really well done on the port and I'm happy when Tim gives a final thumbs up (don't bother uploading again with the changes I've requested, once you do them I'm fine :)

Thanks!

-eric

lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp
453–457 ↗(On Diff #50804)

LLVM style is no braces around single if/if-else statements. Going to be a bit of churn to fix though in your patch.

lib/Target/Lanai/LanaiISelDAGToDAG.cpp
262–263 ↗(On Diff #50804)

Are you actually using these in a way that you should do something with them?

lib/Target/Lanai/LanaiMemAluCombiner.cpp
81 ↗(On Diff #50804)

Probably want TII to match other naming conventions.

285 ↗(On Diff #50804)

Offset should probably be capitalized.

290–292 ↗(On Diff #50804)

Weird that you get them all as pointers and then turn them to references on use? :)

407 ↗(On Diff #50804)

Can probably move this to the top of the function.

lib/Target/Lanai/LanaiRegisterInfo.h
28 ↗(On Diff #50804)

AFAICT you only use this in one function so you could probably just sink it and get it via the MachineFunction.

lib/Target/Lanai/LanaiSelectionDAGInfo.cpp
26–27 ↗(On Diff #50804)

Odd comment when you don't check a subtarget :)

lib/Target/Lanai/LanaiSetflagAluCombiner.cpp
56–58 ↗(On Diff #50804)

You only seem to use this once in the file. Can probably just sink it.

lib/Target/Lanai/LanaiSubtarget.h
17 ↗(On Diff #50804)

Goes after the LLVM includes.

lib/Target/Lanai/LanaiTargetMachine.cpp
108 ↗(On Diff #50804)

I'd probably sink this into the passes themselves. You can set different optimization levels on the function (Attribute::OptimizeNone, for example) and this is just checking the overall module optimization level.

lib/Target/Lanai/LanaiTargetObjectFile.cpp
113 ↗(On Diff #50804)

TM is unused here...

121 ↗(On Diff #50804)

... and so doesn't need to be passed here, which means you don't need it on the class as every other function passes it down.

This revision is now accepted and ready to land.Mar 16 2016, 11:32 PM
t.p.northover accepted this revision.Mar 17 2016, 8:14 AM
t.p.northover edited edge metadata.

I think I'm down to trivia too, and would be happy for you to commit without re-upload. Thanks for the many revisions!

Tim.

lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp
193 ↗(On Diff #50804)

There seems to be no validation of branch distance here (24-bits?).

756 ↗(On Diff #50804)

Global-scope functions should be static to avoid exporting unnecessary symbols unless you really expect them to be accessed from elsewhere (this is just the first one I noticed).

lib/Target/Lanai/InstPrinter/LanaiInstPrinter.cpp
47 ↗(On Diff #50804)

Functions begin with lower case in LLVM (unless there's compelling surrounding code for consistency).

lib/Target/Lanai/LanaiDelaySlotFiller.cpp
231–236 ↗(On Diff #50804)

Can't this be done in the .td file? "let Uses = [RCA]" (or Defs) on the instruction?

This revision was automatically updated to reflect the committed changes.
jpienaar marked 17 inline comments as done.

Updated and submitted.

Thanks for the good suggestions,

Jacques

lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp
453–457 ↗(On Diff #50804)

Used a ASM matcher to look for more of these.

756 ↗(On Diff #50804)

Fixed thanks.

lib/Target/Lanai/LanaiISelDAGToDAG.cpp
262–263 ↗(On Diff #50804)

No, removed. Thanks

lib/Target/Lanai/LanaiSubtarget.h
63 ↗(On Diff #50731)

Missed these.

lib/Target/Lanai/LanaiTargetObjectFile.cpp
121 ↗(On Diff #50804)

Good catch thanks.

step21 added a subscriber: step21.Jul 29 2016, 12:43 PM