This is an archive of the discontinued LLVM Phabricator instance.

[LV] Make LIT test insensitive to basic block numbering
ClosedPublic

Authored by gilr on Apr 23 2017, 1:04 PM.

Details

Summary

This patch is part of D28975's breakdown.

induction.ll encodes the specific (and rather arbitrary) numbers given to predicated basic blocks by the unique naming mechanism, which makes it sensitive to changes in LV's instruction generation order. This patch replaces those specific numbers with a numeric pattern.

Diff Detail

Repository
rL LLVM

Event Timeline

gilr created this revision.Apr 23 2017, 1:04 PM
mssimpso accepted this revision.Apr 24 2017, 8:34 AM

Hi Gil,

While you're working on this, I wonder if you wouldn't mind going further and just making the block names themselves match the regex? Something like:

vector.body:
  %index = phi i32 [ 0, %vector.ph ], [ %index.next, %[[PRED_UDIV_CONTINUE:.+]] ]
  ...
[[PRED_UDIV_CONTINUE]]:
  ...

I've seen this convention in other tests, and it would further harden the test to potential name changes.

This revision is now accepted and ready to land.Apr 24 2017, 8:34 AM
gilr added a comment.Apr 25 2017, 7:16 AM

Hi Matt,

I've seen this convention in other tests, and it would further harden the test to potential name changes.

So I was asking myself the same question while doing this. I ended up keeping the hard-coded names since they carry a meaning (similar to vector.body) and can actually make the test more robust while OTOH BB name schemes don't change so often. But we should indeed try to keep it consistent across all LITs one way or the other (interleaved-accesses-pred-stores also seems to hard-code the names). Michael, what do you think?
Since this patch is part of the vplan breakdown I'll commit it as-is (to clarify that the VPlan patch only affects the numbering) and will follow up in a separate patch to unify the behavior across all LITs to what ever pattern we converge to.

In D32404#736777, @gilr wrote:

Hi Matt,

I've seen this convention in other tests, and it would further harden the test to potential name changes.

So I was asking myself the same question while doing this. I ended up keeping the hard-coded names since they carry a meaning (similar to vector.body) and can actually make the test more robust while OTOH BB name schemes don't change so often. But we should indeed try to keep it consistent across all LITs one way or the other (interleaved-accesses-pred-stores also seems to hard-code the names). Michael, what do you think?
Since this patch is part of the vplan breakdown I'll commit it as-is (to clarify that the VPlan patch only affects the numbering) and will follow up in a separate patch to unify the behavior across all LITs to what ever pattern we converge to.

Sounds good to me.

mkuper edited edge metadata.Apr 25 2017, 10:42 AM

Honestly, I don't feel strongly about this.

I think the most resilient scheme would be something like matching the meaningful part as a part of the regexp, e.g. [[PRED_UDIV_IF1:pred.udiv.if[0-9]+]], [[PRED_UDIV_IF2:pred.udiv.if[0-9]+]], etc. I'm just not sure the overhead is worth it.

This revision was automatically updated to reflect the committed changes.