Page MenuHomePhabricator

[LoopInfo] Don't bail out early in getLoopID
AbandonedPublic

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

Details

Reviewers
sanjoy
hfinkel
Summary

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
getLoopID.

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

lib/Analysis/LoopInfo.cpp
223–225

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.

[1] https://llvm.org/doxygen/classllvm_1_1LoopBase.html#abbefed3df8ce43cf3f0c2f7fd004f6a4

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.

lib/Analysis/LoopInfo.cpp
221

Please reflow this comment

224

Usually we don't use this->

256

Usually we don't use this->.

257

No braces for one stmt blocks

unittests/Analysis/LoopInfoTest.cpp
108

not: capitialize first letter of variable names?

110

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

133

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.