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

[Toma Tabacu]

Replied to inline review comments.


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp:39-40
@@ +38,4 @@
+    markLabelsAsMicroMips();
+  else
+    Labels.clear();
+}
----------------
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.

================
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();
+  }
+}
----------------
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.

http://reviews.llvm.org/D8006

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