[PATCH] ARM: add VLA extension for WoA Itanium ABI

Saleem Abdulrasool abdulras at fb.com
Mon Jun 9 10:18:20 PDT 2014


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:7137
@@ +7136,3 @@
+  case CodeModel::Kernel:
+    BuildMI(*MBB, MI, DL, TII.get(ARM::tBL))
+      .addImm((unsigned)ARMCC::AL).addReg(0)
----------------
Tim Northover wrote:
> Tim Northover wrote:
> > I don't think this is tracking R4 properly. This call is effectively an instruction that should have "R4<imp-use,kill>, R4<imp-def>". Otherwise different phases may assume R4 is unchanged in the interval.
> > 
> > Take a look at any normal call to an "i32 @foo(i32)" for a similar example with R0.
> I think an assert or two in this function might be a good idea. At the very least, "isThumb" (instructions that look fine but have the wrong encoding are *really* nasty to track down). Possibly isWindows as well.
I like both ideas!  Ive had to track down some cases code that looked right but had the wrong encoding - it is nasty to track down :-(.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:7137-7140
@@ +7136,6 @@
+  case CodeModel::Kernel:
+    BuildMI(*MBB, MI, DL, TII.get(ARM::tBL))
+      .addImm((unsigned)ARMCC::AL).addReg(0)
+      .addExternalSymbol("__chkstk")
+      .addReg(ARM::R12, RegState::Implicit | RegState::Define | RegState::Dead);
+    break;
----------------
Saleem Abdulrasool wrote:
> Tim Northover wrote:
> > Tim Northover wrote:
> > > I don't think this is tracking R4 properly. This call is effectively an instruction that should have "R4<imp-use,kill>, R4<imp-def>". Otherwise different phases may assume R4 is unchanged in the interval.
> > > 
> > > Take a look at any normal call to an "i32 @foo(i32)" for a similar example with R0.
> > I think an assert or two in this function might be a good idea. At the very least, "isThumb" (instructions that look fine but have the wrong encoding are *really* nasty to track down). Possibly isWindows as well.
> I like both ideas!  Ive had to track down some cases code that looked right but had the wrong encoding - it is nasty to track down :-(.
For some reason, I thought I had this tracking R4 usage; guess not.  Good catch!

================
Comment at: lib/Target/ARM/ARMInstrInfo.td:5106
@@ +5105,3 @@
+                      [SDNPHasChain, SDNPSideEffect]>;
+let usesCustomInserter = 1, Uses = [R4] in
+  def __CHKSTK : PseudoInst<(outs), (ins), NoItinerary, [(__chkstk)]>;
----------------
Tim Northover wrote:
> Tim Northover wrote:
> > Similarly, this should probably be marked "Defs = [R4]".
> Oh no, "Defs = [SP]" probably. Sorry.
Yeah, making this: Uses = [R4], Defs = [R4, SP] seems more precise.

http://reviews.llvm.org/D4050






More information about the llvm-commits mailing list