[PATCH] D12191: AAP Backend

[Edward Jones via llvm-commits]

edward-jones added a comment.

Hi Tim,

Very sorry, the update notification had become lost in my email. Thank you very much for your comments, I general agree with all of your points so I'll make the appropriate changes and update the patch. I'll definitely look at adding more tests too, since that appears to be an area where we are lacking.

In terms of maintaining the backend; it is a long term project for us at Embecosm. At the moment it is me and Simon Cook who are doing work actively on AAP.

Initially we are looking at doing nightly regression runs of the GCC regression tests, however we want to set up a public buildbot too.

Thank you,
Ed


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:146-147
@@ +145,4 @@
+unsigned AsmPrinter::getPointerSize() const {
+  if (MAI)
+    return MAI->getPointerSize();
+  return TM.getPointerSize();
----------------
t.p.northover wrote:
> I take it this is part of the workaround for function pointers being larger than others, but I didn't quite see how all the pieces fit together in LLVM, and didn't see any tests in this area.
> 
> This feels like a hack though, could you try to explain a bit more why it's needed?
This is unrelated to long function pointers. In DWARF, AAP has an address length of 4 bytes, as the upper bits are used for flag to identify debug, code and data. This change is needed so that we can emit addresses of the correct length.

================
Comment at: lib/Target/AAP/AAP.h:50
@@ +49,3 @@
+static bool inline isImm12(int64_t I) { return (I >= 0) && (I <= 4095); }
+static bool inline isImm16(int64_t I) { return (I >= -32768) && (I <= 65535); }
+
----------------
t.p.northover wrote:
> t.p.northover wrote:
> > Apart from this one you seem to be using isImm for unsigned immediates and isOff for signed ones. 
> > 
> > I'd sort of suggest isUImm and isSImm or something more explicit, but can certainly live with Imm vs Off if it's consistently applied in the backend.
> This also seems misnamed, even if nothing else changes.
The method names correspond to the immediates used in the InstrInfo.d file, hence the use of 'Off' instead of 'SImm'. It definitely makes sense to rename them though. isImm16 was being (ab)used so that we would match both 0xffff when zext and -1 when sext in assembly, and I need to address that in a more elegant way.

================
Comment at: lib/Target/AAP/AAPISelLowering.cpp:69-70
@@ +68,4 @@
+  setOperationAction(ISD::GlobalAddress, MVT::i16,    Custom);
+  setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i1, Expand);
+  setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i8, Expand);
+
----------------
t.p.northover wrote:
> I'm surprised to see so many types in this list (including below) when only MVT::i16 (and possibly MVT::Other) is actually legal.
> 
> The type legaliser mostly just goes about its business without checking these, so you can almost certainly drop most of the i1 & i8 entries.
> 
> I'd also be very surprised if more than one of the MVT::i16 and MVT::Other cases actually applied in most cases. For example a SELECT should never have an MVT::Other input or result.
> 
> It's more complicated for other functions (e.g. setLoadExtAction), so you should expect more of those to be needed.
Yes, I definitely need to clear these up.

================
Comment at: lib/Target/AAP/AAPInstrInfo.td:551-553
@@ +550,5 @@
+let isBranch = 1, isTerminator = 1, isBarrier = 1, usesCustomInserter = 1 in {
+  def BR_CC : Pseudo
+    <(outs), (ins i16imm:$cc, GR64:$lhs, GR64:$rhs, brtarget:$target),
+      "#BR_CC", [(AAPbrcc imm:$cc, GR64:$lhs, GR64:$rhs, bb:$target)]>;
+}
----------------
t.p.northover wrote:
> Why does this need to have a custom inserter? It would seem better to get patterns matching each CC into the actual instructions.
> 
> Generally, custom inserters are a last resort for instructions that need to mangle the CFG (like your select implementation).
I'll have a look at this. I have a feeling there is a reason that we use a custom inserted, and if that's the case at the very least I need to document the reason.


http://reviews.llvm.org/D12191