This is an archive of the discontinued LLVM Phabricator instance.

[SDAG] Fix CombineTo ordering in visitZERO_EXTEND and visitSIGN_EXTEND
ClosedPublic

Authored by niravd on Apr 3 2017, 1:27 PM.

Details

Summary

Reorder DAG updates in visitZERO_EXTEND and visitSIGN_EXTEND to remove insertions of deleted nodes.

This fixes additional related cases in PR32284.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Apr 3 2017, 1:27 PM
niravd updated this revision to Diff 93935.Apr 3 2017, 1:29 PM

Remove unrelated partial edit

jmolloy requested changes to this revision.Apr 4 2017, 12:08 AM
jmolloy added a subscriber: jmolloy.

Hi,

Thanks for submitting this change, but I don't understand it. The phab issue title and summary don't seem to match up, the summary isn't descriptive enough for me to grok what's going on here and you haven't provided a testcase.

Cheers,

James

This revision now requires changes to proceed.Apr 4 2017, 12:08 AM
niravd retitled this revision from Change ARMFeatures to allow Thumb2 instructions into line to [SDAG] Fix CombineTo ordering in visitZERO_EXTEND and visitSIGN_EXTEND.Apr 4 2017, 6:10 AM
niravd edited the summary of this revision. (Show Details)
niravd updated this revision to Diff 94065.Apr 4 2017, 6:35 AM
niravd edited edge metadata.
niravd edited the summary of this revision. (Show Details)

Add missing test cases. This solves most of the skx SelectionDAG errors we've been seeing under O0.

niravd edited the summary of this revision. (Show Details)Apr 4 2017, 6:36 AM

Hi,

Those testcases are too strict. If you just need to check that a testcase doesn't cause an assertion failure, you can avoid any FileCheck checks and put "; REQUIRES: asserts" (I think that's the syntax) to ensure it only gets run on an asserts build.

James

RKSimon added a subscriber: RKSimon.Apr 4 2017, 7:30 AM
RKSimon added inline comments.Apr 4 2017, 7:33 AM
test/CodeGen/X86/pr32284.ll
22 ↗(On Diff #94065)

Remove the ; CHECK- tags - they aren't used.

niravd added a comment.Apr 4 2017, 7:38 AM

This is specifically what Chandler requested on a similar previous patch (see https://reviews.llvm.org/D31286).

Hi,

Those testcases are too strict. If you just need to check that a testcase doesn't cause an assertion failure, you can avoid any FileCheck checks and put "; REQUIRES: asserts" (I think that's the syntax) to ensure it only gets run on an asserts build.

James

Hi Nirav,

Looking at the linked issue, it doesn't look like Chandler actually saw the updated testcase, approving before you updated it.

This testcase is overly strict and any change in codegen will make it fail. If you really want to put FileCheck checks in there, you should check *some property* of the generated output, not it in its entirety.

James

niravd updated this revision to Diff 94079.Apr 4 2017, 7:58 AM
niravd edited the summary of this revision. (Show Details)

Prune tests and kill unused CHECK lines

You're still checking the entire codegen output for this function. That's not what this test is about; this doesn't add any value, all it does is add maintenance burden.

Can the test be reduced at all?

I think, at least for this case, I respectfully disagree with Chandler and don't see the utility in adding FileCheck lines.

James

niravd updated this revision to Diff 94080.Apr 4 2017, 8:04 AM

Strip out FileCheck checks

This is specifically what Chandler requested on a similar previous patch (see https://reviews.llvm.org/D31286).

What Chandler requested in that commit is what James is requesting now. A meaningful check on the generated instructions.

Not nothing. Not everything. A meaningful representation of the problem, which normally translates to a short (3-5) sequence of instructions that change between bugged and non-bugged versions.

Both Chandler and James are absolutely correct that checking for failure as well as checking for the exact sequence will generate problems later and it's not acceptable.

If you don't know what to check for, run the program before and after your patch and see what changes. This will also give confidence to the reviewers that your patch is only changing what's required and needed, not some random parts of the program.

cheers,
--renato

niravd updated this revision to Diff 94104.Apr 4 2017, 12:46 PM

ExtendSetCCs must happen before other CombineTo calls. This should fix the errors dbabokn found

rengolin requested changes to this revision.Apr 4 2017, 2:49 PM

The fact that this review keeps going between all-checks and no-checks is enough to make me distrust the change.

The fact that people are finding errors with the implementation and the tests don't reflect that makes me distrust the tests.

The fact that the description is lacking and there was no further attempt to explain what the problem is makes me distrust the analysis of the problem and therefore the fix.

The fact that bug PR32884 doesn't exist is just the icing.

This revision now requires changes to proceed.Apr 4 2017, 2:49 PM
niravd added a comment.Apr 4 2017, 5:31 PM

Renato, apologies for the frustration. Had I not been in a rush to leave I would have realized my "please don't review yet" message hadn't been committed.

This started as a follow up fix for PR32284 (not 32884 as I typoed in the summary) which is a simple CombineTo ordering issue that caused use of deleted SDNodes causing assertions and could be solved with a little reordering. It was reopened after Dmitry found 100s large number of test failures of which he'd been able to package a few. Having a fix for that It was entirely unclear if I'd gotten them all and started this review as a rendezvous. I hadn't but the initial failure I saw looked unrelated so I planned on a follow up patch when I'd gotten a few more failures. But looking at the new failures this afternoon made it clear there was an ordering dependence on N0 and SetCCs which should be part of this patch.

At this point I'm waiting to hear if the current patch fixes the problem or if there's another testcase that has a similar failure.

I'm still uncertain what to do about tests. The nature of the assertion requires a certain amount of redundance in the input making using the auto-generated path relatively long as James commented. If we limited the tests to -O0 the output it should be fairly stable though and the differences should be obvious (and given these were all found with -O0 doesn't seem unreasonable). The make sure it doesn't assert approach seems reasonable as well.

Barring comments, my default plan is to mark the shared subset of instructions across the RUN calls that make sense.

dbabokin edited edge metadata.Apr 4 2017, 5:52 PM

Nirav,

I still see 19 fails with current patch. All the remaining fails that I have are reduced and attached to 32284 as third attachment.

Thanks for taking care of these fails.

niravd edited the summary of this revision. (Show Details)Apr 4 2017, 6:19 PM

Hi,

Barring comments, my default plan is to mark the shared subset of instructions across the RUN calls that make sense.

As Renato said previously, a sequence of 3-4 CHECK lines is normally more than sufficient. Any more than that and I start to worry about what the test is actually checking for.

James

niravd updated this revision to Diff 94426.Apr 6 2017, 12:59 PM
niravd edited edge metadata.

Update with new test cases now that Dmitry has verified this fixes all of his cases

niravd added a comment.Apr 6 2017, 1:00 PM

This should is now ready to review.

Pinging again.

This revision now requires changes to proceed.Apr 24 2017, 6:08 AM
niravd requested review of this revision.Apr 24 2017, 6:09 AM
niravd planned changes to this revision.May 1 2017, 8:17 AM

It looks like this is still being marked as pending revision. I'm explicitly marking as planning changes and updating it.

niravd updated this revision to Diff 97293.May 1 2017, 8:18 AM

Rebasing.

niravd added a comment.May 9 2017, 6:16 AM

Pinging again. This patch still just needs a review that the test checking is acceptable.

Ping again. This is only waiting on an okay on the tests.

Pinging once more.

niravd edited the summary of this revision. (Show Details)May 30 2017, 8:21 AM
RKSimon added inline comments.May 30 2017, 8:25 AM
test/CodeGen/X86/pr32284.ll
1 ↗(On Diff #97293)

Please can you re-add the i686 tests?

niravd updated this revision to Diff 101065.Jun 1 2017, 11:52 AM

Readd i686 tests.

RKSimon accepted this revision.Jun 1 2017, 12:25 PM

Thanks, LGTM

This revision was automatically updated to reflect the committed changes.