[PATCH] Sparc: Implement i64 load/store support for 32-bit sparc.

Tim Northover t.p.northover at gmail.com
Mon May 18 09:55:37 PDT 2015


================
Comment at: lib/Target/Sparc/SparcFrameLowering.cpp:232-235
@@ -229,3 +231,6 @@
+      unsigned mapped_preg = preg - SP::I0_I1 + SP::O0_O1;
+      MRI.replaceRegWith(preg, mapped_preg);
+    }
     // Mark the reg unused.
     MRI.setPhysRegUnused(reg);
   }
----------------
jyknight wrote:
> t.p.northover wrote:
> > Don't you need to mark the pair unused as well? I wouldn't swear to this, but a quick look suggests that marking I0_I1 unused would implicitly mark I0 and I1 unused, but not the converse.
> As far as I can tell, I0_I1 (etc) don't ever get used bits set on them, so they don't need to be unmarked either.
Ah, that makes sense. Their fundamental Units are already covered in the outer loop.

================
Comment at: lib/Target/Sparc/SparcFrameLowering.cpp:245
@@ +244,3 @@
+      MBB->removeLiveIn(reg);
+      MBB->addLiveIn(reg - SP::I0 + SP::O0);
+    }
----------------
jyknight wrote:
> t.p.northover wrote:
> > Is this delta correct? I'd have expected "reg - SP::I0_I1 + SP::O0_O1" unless I'm misunderstanding the logic.
> This code is indeed *not* correct. Thanks for catching that!! Any idea how to cover that in a test case?
Not sure. It might be visible in "llc -debug", but I'm not sure we encourage tests like that (it would certainly need a "REQUIRES: asserts" line).

================
Comment at: lib/Target/Sparc/SparcISelLowering.cpp:2641-2642
@@ +2640,4 @@
+  EVT MemVT = LdNode->getMemoryVT();
+  if (MemVT == MVT::f128)
+    return LowerF128Load(Op, DAG);
+
----------------
jyknight wrote:
> t.p.northover wrote:
> > I don't understand this: it's the only place a 128-bit type is mentioned at all in the patch. Why does the default action change?
> Not quite the only place, see also LowerSTORE, which has a similar piece of code.
> 
> 
> Note the call to LowerLOAD from LowerOperation: the code there used to say "return LowerF128Load(Op, DAG);", which simply assumed that the type passed in was *always* MVT::f128 when called with ISD::LOAD. That's no longer true, because MVT::i64 is also marked for Custom handling via setOperationAction, so LowerOperation could be called with i64, now too.
> 
> Unlike store, I don't actually need any custom handling for it here (because the custom handling for it occurs in ReplaceNodeResults, instead), but this code does need to not do the f128 stuff for it.
Ah yes, I completely missed the 2-line LowerF128 directly in the switch. Sorry about that.

http://reviews.llvm.org/D8713

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






More information about the llvm-commits mailing list