[PATCH] Don't report the error when non-dangerous load/store is in branch delay slot

Sasa Stankovic Sasa.Stankovic at imgtec.com
Fri Jun 6 17:14:03 PDT 2014


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:151
@@ +150,3 @@
+
+    // Sandbox loads, stores and SP changes.
+    unsigned AddrIdx;
----------------
Mark Seaborn wrote:
> Sasa Stankovic wrote:
> > Mark Seaborn wrote:
> > > I don't get why you've reordered this chunk of code.  Surely the ordering doesn't matter, because Inst can't be both a call and a memory access on MIPS, can it?
> > If for the load/store or stack change instruction the condition (MaskBefore || MaskAfter) is false, then in the original ordering the code would first unnecessary check whether the instruction is call (it isn't), and then would check whether the instruction is in the branch delay slot ("if (PendingCall)"). With the ordering in the patch, the unnecessary check whether the instruction is call is avoided.
> I don't get it.  Surely if you have an instruction for which "IsMemAccess || IsSPFirstOperand" is true but "MaskBefore || MaskAfter" is false, then the "isCall(Inst.getOpcode(), &IsIndirectCall)" check is going to get executed regardless of the ordering of these two chunks of code?  Is that what you mean?
> 
> Can you restore the original ordering to make the correctness fix clearer, please?
> 
> If I've misunderstood this, and the ordering affects compile time (or code clarity), could you do the reordering as a separate change?
What I wanted is basically this: since only the case for load/store or stack change instruction has fall-through path (the cases for jumps and calls both end with a return instruction), I wanted the code that is executed in the fall-through case to be right below it. But OK, I'll restore the original ordering in the patch. As for new ordering, the code should probably first check whether the instruction is load/store (since usually there are more loads/stores than calls or jumps in a function), then check for call and at the end check for indirect jump.

http://reviews.llvm.org/D4048






More information about the llvm-commits mailing list