Page MenuHomePhabricator

[LoopInfo] Don't bail out early in getLoopID

Authored by loladiro on May 4 2018, 1:49 PM.



As far as I understand getLoopID needs to look at the intersection of

  1. BasicBlocks in the loop and
  2. Predecessors of the loop header

and check each of those terminators for a !loop metadata node. However,
what getLoopID is currently doing is iterate through the basic blocks
and require that every one of them have an edge back to the loop header
with the appropriate metadata. This seems incorrect and causes situations
in which a LoopID set with setLoopID cannot be subsequently retrieved with

Diff Detail

Event Timeline

loladiro created this revision.May 4 2018, 1:49 PM
fhahn added a subscriber: fhahn.May 7 2018, 1:31 PM

Could you add a test for that? e.g in unittests/Analysis/LoopInfoTest.cpp


Shouldn't getLoopLatches return exactly the nodes we are interested in (predecessors of the header in the loop) [1]? IIUC using it would simplify the code even further.


loladiro updated this revision to Diff 146485.May 12 2018, 3:12 PM

Updated to use getLoopLatches and add a test.

fhahn added a comment.May 14 2018, 4:12 AM

Thanks for adding the test case! Just a few small comments left.


Please reflow this comment


Usually we don't use this->


Usually we don't use this->.


No braces for one stmt blocks


not: capitialize first letter of variable names?


We can get all the info using LoopInfo I think. An assert that we have 2 latches should be enough?


Could you extend this test case to also check the cases where the latches contain differing ids and where one latch does not contain a loop id, while the other does? It should be possible to just change the attached loop ids I think.

@loladiro bump. It sounds like this was approved, but it doesn't apply cleanly now.

Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2019, 10:16 AM
loladiro abandoned this revision.Apr 26 2019, 3:12 PM
loladiro added a subscriber: jdoerfert.

Looks like this got fixed by @jdoerfert in rL341926 with a near identical patch and test. No rebase required.

Actually, looks like the change to setLoopID is still relevant. I'll submit a new revision with just that change.