[X86] Mark all byval parameters as aliased

[jmorse]

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

[lebedev.ri]

Test?

[jmorse]

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]

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]

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);

[andreadb]

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

[jmorse]

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

[wristow]

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

2 ↗

(On Diff #144567)

'bar' or 'baz'?

19 ↗

(On Diff #144567)

Remove this XXX line?

[spatel]

In D45022#1084793, @wristow wrote:

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]

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]

In D45022#1086594, @jmorse wrote:

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]

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]

LGTM.

[spatel]

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.

lib/Target/X86/X86ISelLowering.cpp

X86TargetLowering::LowerMemArgument(SDValue Chain, CallingConv::ID CallConv,
 
   // FIXME: For now, all byval parameter objects are marked mutable. This can be
   // changed with more analysis.
   // In case of tail call optimization mark all arguments mutable. Since they
   // could be overwritten by lowering of arguments in case of a tail call.
   if (Flags.isByVal()) {
     unsigned Bytes = Flags.getByValSize();
     if (Bytes == 0) Bytes = 1; // Don't create zero-sized stack objects.
-    int FI = MFI.CreateFixedObject(Bytes, VA.getLocMemOffset(), isImmutable);
+    int FI = MFI.CreateFixedObject(Bytes, VA.getLocMemOffset(), isImmutable, true);
     // Adjust SP offset of interrupt parameter.
     if (CallConv == CallingConv::X86_INTR) {
       MFI.setObjectOffset(FI, Offset);
     }
     return DAG.getFrameIndex(FI, PtrVT);
   }
 
   // This is an argument in memory. We might be able to perform copy elision.