[PATCH] D21960: [Sparc] Leon errata fixes passes.

[James Y Knight via llvm-commits]

jyknight added a comment.

I see you've already committed this in the time it took me to write down comments this morning. Please revert, and reopen the review. It seems to me that there are significant issues here. Some of those also present in code in LeonPasses.cpp that already was committed.

Also:

While it's not a big deal in this case, please don't do a file-wide clang-format in the same change as actual changes. You should be doing "git clang-format" or "clang-format-diff" to only format the lines you're modifying. (If the file is really egregiously ill-formatted, you can also run clang-format on the unmodified file, and commit that first).


================
Comment at: lib/Target/Sparc/LeonFeatures.td:65-68
@@ +64,6 @@
+
+def PreventRoundChange
+    : SubtargetFeature<"prvntroundchange", "PreventRoundChange", "true",
+                       "LEON3 erratum fix: Prevent any rounding mode change "
+                       "request: use only the round-to-nearest rounding mode">;
+
----------------
dcederman wrote:
> As jacob_hansen commented, this should only be a warning. It is not a good idea to remove valid function calls without notifying the user.
This doesn't appear to have been addressed.

================
Comment at: lib/Target/Sparc/LeonFeatures.td:81-85
@@ +80,7 @@
+
+def FlushCacheLineSWAP
+    : SubtargetFeature<"flshcachelineswap", "FlushCacheLineSWAP", "true",
+                       "LEON3 erratum fix: Flush cache line containing the "
+                       "lock before performing any of the atomic instructions "
+                       "SWAP and LDSTUB">;
+
----------------
dcederman wrote:
> Flushing the cache is not a valid workaround for this errata, so this feature is not needed and will only decrease performance.
This doesn't appear to have been addressed.

================
Comment at: lib/Target/Sparc/LeonPasses.cpp:263
@@ -265,2 +262,3 @@
         Reg3Index = MI.getOperand(2).getReg();
       } else if (MI.isInlineAsm()) {
+        StringRef AsmString =
----------------
All of the isInlineAsm branches -- new and old -- seem wrong.

Testing instructions by looking at the prefix of the asm string can't possibly work properly. E.g., it's completely wrong if there are multiple instructions in an inline asm string.

One of two things makes sense. Either:
a) these fixes are only intended to be done on compiler-generated code. If so just remove the isInlineAsm clauses.

b) it's supposed to be done at translation of asm into object file. In which case, it should be done for an actual assembly input file too and these transforms are misplaced here.

I don't know myself how to introduce a transform at the asm level; there doesn't seem to be an obvious way to do it. Please ask for advice on the mailing list if that's what you need to do.


================
Comment at: lib/Target/Sparc/SparcISelLowering.cpp:1642
@@ -1641,3 +1638,3 @@
-  else if (Subtarget->hasLeonCasa())
+  if (Subtarget->isV9() || Subtarget->hasLeonCasa())
     setMaxAtomicSizeInBitsSupported(64);
   else
----------------
This should actually have been 32. 64bit atomic instructions are not supported on leon. Also, unrelated to this change.

================
Comment at: llvm/trunk/lib/Target/Sparc/SparcISelLowering.cpp:3380
@@ -3384,3 +3379,3 @@
     switch (Constraint[0]) {
-    default:  break;
-    case 'r': return C_RegisterClass;
+    default:
+      break;
----------------
Unrelated change.

================
Comment at: llvm/trunk/lib/Target/Sparc/SparcISelLowering.cpp:3458
@@ -3459,1 +3457,3 @@
     switch (Constraint[0]) {
+    case 'f':
+      return std::make_pair(0U, &SP::FPRegsRegClass);
----------------
Unrelated change.


Repository:
  rL LLVM

http://reviews.llvm.org/D21960