[PATCH] Mark alias symbols as microMIPS if necessary.
Matheus Almeida
matheus.almeida at imgtec.com
Mon Mar 17 05:22:34 PDT 2014
Hi,
I can see that this patch follows roughly what has been done to allow labels to be marked as micromips. There is however a small and important difference that we shouldn't overlook. The problem I see here is that you are only adding a call to the target hook from the object code path and are ignoring the 'Asm' code path. Was this on purpose ?
I don't think this is a violation of the MC Layer design per se because you have in fact implemented emitAssignment for all the target streamers but I think you should cover both code paths.
For example, you could:
1) Update MCAsmStreamer::EmitAssignment and add logic to call the target hook.
2) Add an implementation of EmitAssignment to MCStreamer that the only thing it does is to call the target hook. Each implementation of the MCStreamer could then override EmitAssignment but they would need to call the base implementation (MCStreamer::EmitAssignment) so that the target hook is invoked for both code paths.
Regards,
Matheus
================
Comment at: include/llvm/MC/MCStreamer.h:82
@@ -81,1 +81,3 @@
virtual void emitLabel(MCSymbol *Symbol);
+ // Allow a target to add behavior to the of emitAssignment MCStreamer.
+ virtual void emitAssignment(MCSymbol *Symbol, const MCExpr *Value);
----------------
Grammar =]
================
Comment at: test/MC/Mips/alias_micromips.s:1
@@ +1,2 @@
+# RUN: llvm-mc -filetype=obj -triple mipsel-unknown-linux -mcpu=mips32 %s -o - \
+# RUN: | llvm-readobj -t | FileCheck %s
----------------
It looks like this file has the executable bit set which isn't needed as far as I know.
================
Comment at: lib/MC/MCObjectStreamer.cpp:189-191
@@ -188,1 +188,5 @@
+
+ MCTargetStreamer *TS = getTargetStreamer();
+ if (TS)
+ TS->emitAssignment(Symbol, Value);
}
----------------
See general comment.
http://llvm-reviews.chandlerc.com/D3080
More information about the llvm-commits
mailing list