[PATCH] D6231: [mips] Expand JAL instructions when PIC is enabled.

[Daniel Sanders via llvm-commits]

dsanders commandeered this revision.
dsanders edited reviewers, added: tomatabacu; removed: dsanders.
dsanders added a comment.
This revision is now accepted and ready to land.

Commandeered since Toma has finished his placement with us.

> > In http://reviews.llvm.org/D6231#191632, @dsanders wrote:

>  > That's ok for this patch, but following the principle above I'd suggest asserting that LargeGOT is not in use.

>  > I just realized that llvm-mc doesn't even accept -mxgot on the command line (it's also not listed in --help-hidden).

> 

> It seems that -mxgot is a hidden option only for llc.


We'll need to add -mxgot in a later patch. For now, I'll just add a fixme.

> > Didn't we establish earlier in the review that these worked as expected? Has that changed?

>  I'm not sure we did. We're still treating forward-declared local symbols as global, while GAS treats them as local.

>  Just to be clear, I'm talking the following:

> 

> .option pic2

> 

>   jal foo

> 

> foo:


I've decided not to worry about the handling of forward-declared local symbols. If we do still have issues, then we can fix them later. For now, it's more important to get Toma's remaining work committed.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1468-1473
@@ +1467,8 @@
+
+    // We can do this expansion if there's only 1 symbol in the argument
+    // expression.
+    if (countMCSymbolRefExpr(JalExpr) > 1)
+      return Error(IDLoc, "jal doesn't support multiple symbols in PIC mode");
+
+    const MCSymbol *JalSym = getSingleMCSymbol(JalExpr);
+
----------------
tomatabacu wrote:
> dsanders wrote:
> > I'm not sure I understand this bit. Can you explain?
> GAS spews all kinds of errors if it is given 2 symbols here, so we should report an error as well.
> Also, if we didn't, the IAS would (currently) segfault.
I understand that Toma was checking for strange assembly like:
  jal foo-bar
I agree that this should have a nice error message, but I wonder if this patch is checking in the right place. I think it may be a job for the code that figures out which reloc to use for the expression.

Having looked into this a bit, I think that handling this in the right place is related to representing our unary operators (%hi(), etc.) actual operators in MCExpr nodes. printExpr() ought to blindly print the expression it has whether valid or not, and the reloc selection should handle validity.

For now, I'll add a FIXME comment saying it should be removed once %hi(), etc are MCExpr operators.


http://reviews.llvm.org/D6231