[PATCH] [ARM] Implement eabi_attribute, cpu, and fpu directives.

Renato Golin renato.golin at linaro.org
Tue Oct 22 11:23:00 PDT 2013


  Thanks Logan, looks a lot better. I'm happy when Richard is happy.


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:125
@@ +124,3 @@
+void ARMTargetAsmStreamer::emitTextAttribute(unsigned Attribute,
+                                             StringRef String) {
+  switch (Attribute) {
----------------
Logan Chien wrote:
> Renato Golin wrote:
> > A better name for this would be CPU
> For now, this function is only for CPU.  I am fine to rename this function.  However, I am not sure whether it is a good idea or not.  According to the specification, there might be other text attributes, such as Tag_CPU_raw_name, Tag_CPU_name, Tag_compatibility, Tag_also_compatible_with, and Tag_conformance.  I can do the work if you feel that emitCPU() is better.
Oh, my bad, ignore.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:166
@@ +165,3 @@
+      Value >>= 7;
+      Size += sizeof(int8_t); // Is this really necessary?
+    } while (Value);
----------------
Logan Chien wrote:
> Renato Golin wrote:
> > I don't think so.
> This function is copied from AttrEmitter (including the comment).  I think we can leave this function unchanged for now.  I am planning to move getULEB128Size() from MCAsmInfo to llvm/Support/LEB128.h, and remove this function in the future.
Agree.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:472
@@ +471,3 @@
+  setAttributeItem(Attribute, Value);
+}
+void ARMTargetELFStreamer::emitFPU(StringRef FPU) {
----------------
Logan Chien wrote:
> Renato Golin wrote:
> > These two functions look redundant. Is it just in ARM's case?
> Sorry.  I don't understand what do you mean by redundant.  These are virtual functions used by the AsmPrinter and AsmParser.  May you further describe your question?  Thanks.
It might just be my lack of contact with this code for a while, but this looks like it's just adding two aliases to the overloaded "setAttributeItem" function.

My question was related to the need to have this extra level of indirection if they're just calling another function, or if it was better to, either expose the other two, or rename them to "emitAttribute" and "emitTextAttribute".

This may not be the case with other back-ends, so I'm not sure my question is at all relevant.


http://llvm-reviews.chandlerc.com/D1987



More information about the llvm-commits mailing list