This is an archive of the discontinued LLVM Phabricator instance.

[X86] Mark all byval parameters as aliased
ClosedPublic

Authored by jmorse on Mar 29 2018, 6:09 AM.

Details

Summary

The formal LLVM IR Value * to the function is guaranteed to alias this stack slot, thus later rescheduling optimisations of accesses to this stack slot may be aliased and unsafe. See: PR30290

Diff Detail

Event Timeline

jmorse created this revision.Mar 29 2018, 6:09 AM

Hi, more sharing it to make clear in https://bugs.llvm.org/show_bug.cgi?id=30290 what the solution may be. I figured I'd add reviewers if it was agreed to be a fix, but if this kind of discussion-patch should go somewhere else, do let me know.

jmorse updated this revision to Diff 141811.Apr 10 2018, 2:58 AM
jmorse added a reviewer: craig.topper.

Added regression test and craig.topper as reviewer; Hi Craig, this is a fix for https://bugs.llvm.org/show_bug.cgi?id=30290 which has gone quiet. The tl;dr is that without isAliased=true for byval argument stack slots, the instruction scheduler doesn't know the byval stack slot is aliased by an IR Value, and can illegally re-order frameindex-based and Value-based memory accesses of the same location, as demonstrated in PR and added regression test.

wristow added inline comments.Apr 24 2018, 6:51 PM
lib/Target/X86/X86ISelLowering.cpp
2836

The code-change itself looks safe to me, and my understanding of the discussion in http://llvm.org/PR30290 is that this approach is conservative in terms of the aliasing that it will mark (but that identifying the precisely-minimal-aliasing will require a very large amount of restructuring). My guess is that this conservative aliasing won't have a serious impact on overall code quality. Like the FIXME comment above (line 2829), I think similar comment would be good. Something like:

// FIXME: For now, all byval parameter objects are marked as aliasing. This
// can be improved with deeper analysis.

But I have to say I don't have a deep understanding of the area, so I'm curious to hear what others think.

Formatting nit: With the added true parameter, the line is more than 80 characters.
Also, I'd prefer a comment indicating what the parameter being set to true is doing:

int FI = MFI.CreateFixedObject(Bytes, VA.getLocMemOffset(), isImmutable,
                               /*isAliased=*/true);

Could you please add an llc test instead of an lli test?

jmorse updated this revision to Diff 144567.Apr 30 2018, 8:37 AM

Updated with comment + cosmetics to the CreateFixedObject call, changed test to be an llc codegen test.

A couple minor test-tweak comments in-line.
As before, the code-change looks safe and conservative to me, in terms of the aliasing. And my guess is that conservative aspect is fine, in that it won't seriously impact performance. So I'm happy to say LGTM. Does anyone with more experience in this area have any concerns?

test/CodeGen/X86/pr30290.ll
3

'bar' or 'baz'?

20

Remove this XXX line?

spatel added a comment.May 2 2018, 8:46 AM

A couple minor test-tweak comments in-line.
As before, the code-change looks safe and conservative to me, in terms of the aliasing. And my guess is that conservative aspect is fine, in that it won't seriously impact performance. So I'm happy to say LGTM. Does anyone with more experience in this area have any concerns?

I don't have any better suggestions about the code change, but let me chime in with a general comment about the test: it would be better to commit the test in trunk without this code change, so we have the baseline (miscompile). That way, if there are any temptations to revert the code change because of a perf problem, it will be clear that we would be reintroducing a miscompile.

Also, please use utils/update_mir_test_checks.py or utils/update_llc_test_checks.py to generate the CHECK lines. That's better than adding assertions by hand in almost all cases.

jmorse updated this revision to Diff 145031.May 3 2018, 9:27 AM

Improves comments in test description, use utils/update_llc_test_checks.py to generate CHECK lines in test.

@spatel just to confirm I understand you correctly, you're saying to commit the test first (where it would fail) followed by the code change that fixes it, to discourage unconsidered reversion, yes?

spatel added a comment.May 3 2018, 9:44 AM

Improves comments in test description, use utils/update_llc_test_checks.py to generate CHECK lines in test.

@spatel just to confirm I understand you correctly, you're saying to commit the test first (where it would fail) followed by the code change that fixes it, to discourage unconsidered reversion, yes?

Correct. You can commit the test right now (with auto-generated CHECK lines that show the bug). Add a FIXME comment too to be extra clear that the output shown is a miscompile.
After that commit, apply your code fix locally, rebuild llc, and regenerate the CHECK lines using the script. There will only be a couple of lines changing in the test file now. Upload the complete rebased diff here. Then, we'll have a patch that clearly shows that the miscompile that was present will be fixed with this patch.

jmorse updated this revision to Diff 145167.May 4 2018, 3:36 AM

Rebase against now-committed test case. The patch against the test is indeed much clearer now, it's obvious what the effect of the code change is.

spatel accepted this revision.May 4 2018, 7:02 AM

LGTM.

This revision is now accepted and ready to land.May 4 2018, 7:02 AM

Note: I marked this patch as 'accepted' on Phab over an hour ago, but I
haven't gotten an email about it, so sending 'LGTM' via email.

This revision was automatically updated to reflect the committed changes.