[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] code refactoring on ARMTargetInfo class
From:       Renato Golin <renato.golin () linaro ! org>
Date:       2015-06-30 20:27:22
Message-ID: e855499a8f306781841e6a2da61a7550 () localhost ! localdomain
[Download RAW message or body]

Hi Alexandros,

Thanks for the patch. I'm really happy that we can now change everything else and not \
need to change the TargetParser any more. This means we're stabilising the interface, \
and soon will be time to move it inside the new Tuple class.

About your patch, it's overall good, but I have a few minor comments inline.

cheers,
--renato


REPOSITORY
  rL LLVM

================
Comment at: lib/Basic/Targets.cpp:4097
@@ +4096,3 @@
+    StringRef ArchName = Triple.getArchName();
+    unsigned  ArchISA  = llvm::ARMTargetParser::parseArchISA(ArchName);
+
----------------
These could be cached, too, as members of the class.

================
Comment at: lib/Basic/Targets.cpp:4101
@@ +4100,3 @@
+    ArchProfile = llvm::ARMTargetParser::parseArchProfile(ArchName);
+    ArchVersion = llvm::ARMTargetParser::parseArchVersion(ArchName);
+
----------------
What if this goes wrong? 

I guess all other methods in the class will start to return rubbish, but then again, \
that's what it currently does.

Maybe a future patch to cover the cases where the values are XX_INVALID on all the \
get methods and print some warning/error.

Also, you might want to extract this lines (and IsThumb / Atomic below) into a \
private method that both the constructor and setCPU call.

================
Comment at: lib/Basic/Targets.cpp:4338
@@ +4337,3 @@
+    ArchProfile = llvm::ARMTargetParser::parseArchProfile(ArchName);
+    ArchVersion = llvm::ARMTargetParser::parseArchVersion(ArchName);
+ 
----------------
You should also set IsThumb and ShouldUseInlineAtomic

================
Comment at: lib/Basic/Targets.cpp:4489
@@ +4488,3 @@
+                       (ArchVersion == 5 && CPUAttr.count('E')));
+    bool is32Bit = (!IsThumb || supportsThumb2(CPUAttr));
+    if (is5EOrAbove && is32Bit && (CPUProfile != "M" || CPUAttr  == "7EM"))
----------------
This variable name is odd...

http://reviews.llvm.org/D10839

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



_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]