<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 17, 2017 at 8:39 AM, Violeta Vukobrat via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" class="m_8968230339695569647cremed cremed" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">violetav added inline comments.<br>
<span><br>
<br>
================<br>
Comment at: include/llvm/CodeGen/MachineBa<wbr>sicBlock.h:781<br>
+ // block's entry. It is added to IncomingCFAOffset in CFIInfoVerifier.<br>
+ int AdjustIncomingCFAOffset = 0;<br>
+ // Amount used for adjusting the value of outgoing cfa offset. This value<br>
----------------<br>
</span><span>iteratee wrote:<br>
> I don't follow the comment about why these are necessary again.<br>
> Can you elaborate?<br>
</span>I will try. So, let's say there is a function that looks like this:<br>
<br>
BB_0<br>
|<br>
BB_1 <-----|<br>
/ \ |<br>
BB_2 BB_3 |<br>
| |<br>
BB_4 |<br>
| |<br>
BB_5 ___|<br>
<br>
and that the following is true:<br>
<br>
1. BB_1 has following cfi directives:<br>
<br>
BB_1:<br>
...<br>
.cfi_adjust_cfa_offset 4<br>
...<br>
.cfi_adjust_cfa_offset -4<br>
...<br>
<br>
2. BB_4 has the same:<br>
<br>
BB_4:<br>
...<br>
.cfi_adjust_cfa_offset 4<br>
...<br>
.cfi_adjust_cfa_offset -4<br>
...<br>
<br>
3. BB_1 is a successor of BB_5 (if it's not clear from the 'illustration', BB_0 has one successor, BB_1 has two, and BB_3,BB_4,BB_5 each have one).<br>
<br>
4. For this example, first the cfi_adjust_cfa directives with positive values are inserted (in both BB_1 and BB_4) and then the ones with negative values.<br></blockquote><div><br></div><div>Feel free to add asserts, but I think the answer needs to be that you can't call updateCFIInfoSucc when the CFI state is inconsistent.</div><div>What is the correct thing to do propagate in this simpler case:</div><div>BB1:</div><div>if (cond) {</div><div>BB2:</div><div> .cfi_adjust_cfa_offset 4</div><div>}</div><div>BB3:</div><div><br></div><div>Should you adjust BB3 or not?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
What happens here with the previous approach is that, although BB_1 and BB_4 do not change cfa offset (their incoming and outgoing offsets are the same), they and their successors end up having the wrong offset set.<br>
<br>
This was the way that in/out offset was set to successors in the previous approach (from updateCFIInfoSucc()):<br>
<br>
// the difference remains the same, calculate it by using previous in/out offset values<br>
int AdjustAmount = Succ->getOutgoingCFAOffset() - Succ->getIncomingCFAOffset();<br>
// set outgoing offset to be sum of the outgoing offset of the predecessor and the amount that this successor adjusts the offset itself (with adjust_cfa_directives)<br>
Succ->setOutgoingCFAOffset(Cur<wbr>rSucc->getOutgoingCFAOffset() + AdjustAmount);<br>
// set incoming offset to be the outgoing offset of the predecessor<br>
Succ->setIncomingCFAOffset(Cur<wbr>rSucc->getOutgoingCFAOffset())<wbr>;<br>
<br>
Let's say that all blocks have in/out offset set to zero at the beginning (just to simplify the numbers).<br>
When first 'adjust_cfa_offset 4' is inserted in BB_1, after updating the successors, the in/out cfa offsets of BBs in the function look like this:<br></blockquote><div><br></div><div>Except, you can't run the propagate algorithm at this point at all. There is no correct answer, even if you did it from scratch. BB_1 has 2 predecessors with different cfa_offset</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
BB_0, in 0, out 0<br>
BB_1, in 0, out 4<br>
BB_2, in 4, out 4<br>
BB_3, in 4, out 4<br>
BB_4, in 4, out 4<br>
BB_5, in 4, out 4<br>
<br>
Next, when 'adjust_cfa_offset 4' is inserted in BB_4:<br></blockquote><div> <br></div><div><br></div><div>Same, it's illegal to run the propogate algorithm here.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
BB_0, in 0, out 0<br>
BB_1, in 8, out 12<br>
BB_2, in 12, out 12<br>
BB_3, in 12, out 12<br>
BB_4, in 4, out 8<br>
BB_5, in 8, out 8<br>
<br>
Next, when 'adjust_cfa_offset -4' is inserted in BB_1:<br></blockquote><div><br></div><div>Still illegal</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
BB_0, in 0, out 0<br>
BB_1, in 8, out 8<br>
BB_2, in 8, out 8<br>
BB_3, in 8, out 8<br>
BB_4, in 8, out 12<br>
BB_5, in 12, out 12<br>
<br>
Next, when 'adjust_cfa_offset -4' is inserted in BB_4:</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
BB_0, in 0, out 0<br>
BB_1, in 8, out 8<br>
BB_2, in 8, out 8<br>
BB_3, in 8, out 8<br>
BB_4, in 8, out 8<br>
BB_5, in 8, out 8<br>
<br></blockquote><div><br></div><div>Now that it's legal, you should propagate the results.</div><div>If you have a pass that's inserting directives in a way that you can't propagate the results after you're done with a block, then you can keep a list of modified blocks and propagate them when you're all finished, or just recompute from scratch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
And then you end up having wrong in/out cfa offset for blocks 1-5 because the adjustments are contained in OutgoingCFAOffset which is the value that gets propagated further. That is what is propagated to the successors, instead of the increment/decrement from adjust directives. This way of calculating in/out cfa offset values produces wrong results for the case where you have a cycle (like in the example above).<br>
<br></blockquote><div><br></div><div>The fact that the code you wrote previously didn't work right isn't a good argument that you need the extra storage in every MBB.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
What is changed now is that each 'adjust_cfa_offset' directive is propagated to the successors when it's inserted and updates their incoming and outgoing adjust values appropriately. In the end, before verifying CFI info and inserting additional CFI instructions if needed, these values are added to incoming and outgoing cfa offset (that now contain only absolute offsets that are set).<br>
<span><br>
<br>
================<br>
Comment at: lib/CodeGen/TargetPassConfig.c<wbr>pp:772<br>
<br>
</span>+ // Verify basic block incoming and outgoing cfa offset and register values.<br>
+ addPass(createCFIInfoVerifier(<wbr>));<br>
----------------<br>
<span>iteratee wrote:<br>
> I thought part of the problem was that these passes should not be run generally.<br>
</span>I put the added target hook in the passes themselves. Should I move it to TargetPassConfig?<br>
<div class="m_8968230339695569647HOEnZb"><div class="m_8968230339695569647h5"><br>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D35844" rel="noreferrer" class="m_8968230339695569647cremed cremed" target="_blank">https://reviews.llvm.org/D3584<wbr>4</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div></div>