[PATCH] [mips] [IAS] Add support for the .insn directive.

[Daniel Sanders]

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp:39-40
@@ +38,4 @@
+    markLabelsAsMicroMips();
+  else
+    Labels.clear();
+}
----------------
tomatabacu wrote:
> dsanders wrote:
> > tomatabacu wrote:
> > > dsanders wrote:
> > > > We should unconditionally clear Labels as the previous code did. We don't need to mark the same labels multiple times.
> > > Labels actually is unconditionally cleared: in markLabelsAsMicroMips() for the if and directly for the else.
> > Good point. For consistency, could you pull it out of markLabelsAsMicroMips() into the two callers or sink the isMicroMipsEnabled() checks inside the markLabelsAsMicroMips() as an early-exit? Both options make the usage consistent and neither is better than the other. If you choose the latter though, you ought to change the name accordingly since it won't touch the labels if micromips is not enabled.
> "could you pull it out of markLabelsAsMicroMips() into the two callers"
> No, because Labels can not be directly accessed from MipsTargetELFStreamer::emitDirectiveInsn(), because it's a private member of MipsELFStreamer.
> 
> "or sink the isMicroMipsEnabled() checks inside the markLabelsAsMicroMips() as an early-exit?"
> I would personally prefer conditionally calling a function that will execute rather than unconditionally calling a function that may or may not execute.
> Also, I can't think of a name for markLabelsAsMicroMips() with the microMIPS check inside which has a reasonable length.
> > "or sink the isMicroMipsEnabled() checks inside the markLabelsAsMicroMips() as an early-exit?"
> I would personally prefer conditionally calling a function that will execute rather than unconditionally calling a function that may or may not execute.

There's nothing particularly wrong with a function doing nothing when it has nothing to do. In this case it wouldn't quite do nothing though. It would still empty the list of labels regardless of whether it annotated the labels or not.

> Also, I can't think of a name for markLabelsAsMicroMips() with the microMIPS check inside which has a reasonable length.

The best name I can think of is createPendingLabelRelocs().

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp:641-644
@@ +640,6 @@
+  // FIXME: Also mark labels when in MIPS16 mode.
+  if (isMicroMipsEnabled()) {
+    MipsELFStreamer &MEF = static_cast<MipsELFStreamer &>(Streamer);
+    MEF.markLabelsAsMicroMips();
+  }
+}
----------------
tomatabacu wrote:
> dsanders wrote:
> > I've just noticed that the Labels.clear() is missing here.
> It's not missing, it's in markLabelsAsMicroMips(). We can't directly access Labels from here, because it's a private member of MipsELFStreamer.
> It's not missing, it's in markLabelsAsMicroMips().

I mean it's missing from the else path. Sorry for being unclear.

The case I have in mind is something like:
  .set nomicromips
  foo:
    .insn
    .4byte 0
  .set micromips
  bar:
    addiu $1, $2, 3
I think the addiu will cause foo and bar to be marked microMIPS when it should only mark bar. This is because because the .insn didn't clear the list.

> We can't directly access Labels from here, because it's a private member of MipsELFStreamer.

A public member for clearing the labels list would fix that but we won't need that if markLabelsAsMicroMips is called unconditionally and always clears the list regardless of whether it marks the labels or not.

http://reviews.llvm.org/D8006

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/