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
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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.
Comment Actions
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.
| 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. int FI = MFI.CreateFixedObject(Bytes, VA.getLocMemOffset(), isImmutable,
/*isAliased=*/true); | |
Comment Actions
Updated with comment + cosmetics to the CreateFixedObject call, changed test to be an llc codegen test.
Comment Actions
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? | |
Comment Actions
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.
Comment Actions
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?
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
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:
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);