This is an archive of the discontinued LLVM Phabricator instance.

[Hexagon] [Test] Remove undef and infinite loop from test
ClosedPublic

Authored by hermord on Aug 18 2018, 7:16 PM.

Details

Summary

As suggested in D50222, this has been refactored into a separate patch.

The undef and the infinite loop at the end cause this test to be translated unpredictably. In particular, the checked-for mpy disappears under certain legal optimizations (e.g. the one in D50222). Since the use of these constructs is not relevant to the behavior tested, according to the header comment, this change, suggested by @kparzysz, eliminates them.

Diff Detail

Repository
rL LLVM

Event Timeline

hermord created this revision.Aug 18 2018, 7:16 PM
kparzysz accepted this revision.Aug 20 2018, 5:37 AM
This revision is now accepted and ready to land.Aug 20 2018, 5:37 AM

Right, this is accepted, i'll commit this for you then.

This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Aug 30 2018, 3:03 AM

And reverted in rL341049.
Does this actually represent the llc output as of svn trunk?
Or is this dependent on D50222?

This revision is now accepted and ready to land.Aug 30 2018, 3:03 AM
hermord updated this revision to Diff 163458.EditedAug 30 2018, 6:49 PM

After a rebase and bisect, it turned out that the current form does rely on D50222. The extra mpy nodes come from this combine on the srem, which would not be reached with the proposed SREM optimization:

Combining: t11: i32 = srem t5, Constant:i32<3>
Creating constant: t36: i32 = Constant<1431655766>
Creating new node: t37: i32 = mulhs t5, Constant:i32<1431655766>
Creating new node: t38: i32 = mul t5, Constant:i32<0>
Creating new node: t39: i32 = add t37, t38
Creating constant: t40: i32 = Constant<31>
Creating new node: t41: i32 = srl t39, Constant:i32<31>
Creating new node: t42: i32 = add t39, t41
Creating new node: t43: i32 = mul t42, Constant:i32<3>
Creating new node: t44: i32 = sub t5, t43
 ... into: t44: i32 = sub t5, t43

Without the other changes in this patch, the srem was discarded and this did not happen.

This change fixes that: though there may be multiple mpys in the loop body, there is only one cmp; looking at it instead still fullfils the purpose of the test (ensuring the loop body is not duplicated), unless I'm mistaken.

@kparzysz this is still ok after the last change?

@kparzysz this is still ok after the last change?

Yes, I believe so. Tests pass successfully with this and this seems conceputally correct to me,

@kparzysz this is still ok after the last change?

Yes, I believe so. Tests pass successfully with this and this seems conceputally correct to me,

Oops my bad, misread the @. Please disregard.

I'm going to assume this is still ok and reland this.

kparzysz added inline comments.Sep 11 2018, 6:21 AM
test/CodeGen/Hexagon/swp-const-tc2.ll
10 ↗(On Diff #163458)

It's the multiplication at line 25 that should only occur once. Could you change these two lines to

; CHECK: r{{[0-9]+}} = mpyi
; CHECK-NOT: r{{[0-9]+}} = mpyi
This revision was automatically updated to reflect the committed changes.