This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Correct t2UMAAL Description
ClosedPublic

Authored by samparker on Jul 1 2016, 9:03 AM.

Details

Summary

Green dragon buildbot caught an error:

http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_build/22367/

During the refactor of multiplication instructions in my previous commit, I missed updating t2UMAAL and there was no test to catch the failure.

commit r274347 was reverted so I have now updated the patch from D21549 to include the correction, plus the additional tests.

Diff Detail

Event Timeline

samparker updated this revision to Diff 62499.Jul 1 2016, 9:03 AM
samparker retitled this revision from to [ARM] Correct t2UMAAL Description.
samparker updated this object.

So the actual change is that we recognize the "$src = $dst" constraint? The test doesn't actually check that (and in fact seems like it would pass right now).

well this test is just to make sure we can generate the instruction without the compiler crashing.

Sure, it's not useless, but it's not testing what's actually changing in this patch either.

Ok, good point. So you would suggest to make another test similar to MACLongTest5?

Yep, that looks like it covers it.

ok, cheers. revision to follow

samparker updated this object.Jul 1 2016, 12:30 PM
samparker updated this revision to Diff 62521.Jul 1 2016, 12:33 PM
samparker updated this object.

Could someone review this please? The only change over D21549 is the test file changes and the one line change (line 2575) for the t2UMAAL description.

Sam, isn't this the same patch as D21549?

Basically yes, but with a more robust test case and the correction to UMAAL which caused the buildbots to fail. The original commit was reverted so I've corrected the issues and updated the original.

Ah, yes. Can you add the new patch to that review as well, so we can get a diff on just what makes the test work?

ping. i updated the original ticket.

rengolin accepted this revision.Jul 19 2016, 6:25 AM
rengolin added a reviewer: rengolin.

This looks reasonable to me, though I forgot what the breakage was in the buildbot. Tests look better, though.

lib/Target/ARM/ARMInstrThumb2.td
2575

This seems to be the only new change...

This revision is now accepted and ready to land.Jul 19 2016, 6:25 AM
This revision was automatically updated to reflect the committed changes.