Don't pass NoAlias attribute for arguments passed by value

[GuillaumeGomez]

We found out about this in rust-lang/rustc_codegen_gcc#312.

You can find more information about this in:

• https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171218/512066.html
• https://reviews.llvm.org/D40118

[rustbot]

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

[GuillaumeGomez]

Just in case:

@bors try @rust-timer queue

[bors]

⌛ Trying commit 32a8d75 with merge 17f23b92274eee4b3eba2dee1fff8264edf3d266...

[nikic]

This seems to be missing test coverage.

To clarify, this is just a stylistic change, right? Applying noalias to byval arguments should be safe, just redundant.

[erikdesjardins]

The linked patch is unrelated to this change. The patch allows more noalias when an argument aliases with a byval argument, whereas this change is removing noalias. (Also that patch is about noalias applied to non-byval arguments, whereas this change is for noalias applied to byval arguments...)

[GuillaumeGomez]

Ah indeed, it links to callsite, not function args.

[erikdesjardins]

That's not the distinction. noalias has the same meaning whether it's applied on the callsite's argument or the function declaration's argument, and the code you're changing affects both.

To put it more simply:

• that patch is saying "noalias is okay in more places"
• your change, by removing noalias, is saying "noalias is okay in fewer places"

Now, this isn't a direct contradiction, since they're also talking about different situations. 🙃 It's just unrelated.

[erikdesjardins]

It's redundant inside the callee for sure, but is it redundant in the caller? Couldn't AA use a call to @foo(ptr %x, ptr byval noalias %y) to conclude that %x is not used to write to %y during the call, in the case where (otherwise) %x and %y MayAlias?

[bors]

☀️ Try build successful - checks-actions
Build commit: 17f23b92274eee4b3eba2dee1fff8264edf3d266 (17f23b92274eee4b3eba2dee1fff8264edf3d266)

[GuillaumeGomez]

This seems to be missing test coverage.

I have no clue what to test here. Just that the ASM generated is the same?

To clarify, this is just a stylistic change, right? Applying noalias to byval arguments should be safe, just redundant.

Normally it shouldn't change anything as LLVM seems to correctly handle this case internally unlike GCC.

[erikdesjardins]

I have no clue what to test here.

A test for this would be that noalias doesn't appear in the LLVM IR for byval arguments.

Normally it shouldn't change anything as LLVM seems to correctly handle this case internally unlike GCC.

Can you elaborate on this? I can think of two possible meanings:

• "LLVM doesn't error, whereas gcc does, when you put noalias (resp. restrict) on a byval argument"
• "noalias is meaningless on byval in LLVM" (I don't think this is true, as I wrote above, but that's rare and not currently exploited by AA so it's 95% true I suppose)

[GuillaumeGomez]

A test for this would be that noalias doesn't appear in the LLVM IR for byval arguments.

Sounds good to me.

Can you elaborate on this? I can think of two possible meanings:

* "LLVM doesn't error, whereas gcc does, when you put `noalias` (resp. restrict) on a `byval` argument"

* "`noalias` is meaningless on `byval` in LLVM" (I don't think this is true, as I wrote [above](https://github.com/rust-lang/rust/pull/115350#issuecomment-1697877362), but that's rare and not currently exploited by AA so it's 95% true I suppose)

I meant:

"LLVM doesn't error, whereas gcc does, when you put noalias (resp. restrict) on a byval argument"

[erikdesjardins]

Okay, that's not the one I was expecting 😁

I don't understand why you're making this change then. If the motivation is that GCC is incompatible with this attribute, then the GCC backend should ignore it, as you've already done in rust-lang/rustc_codegen_gcc#312. It doesn't make sense to remove it entirely from all backends, as this PR does, unless it's actually wrong to specify it. (Which I don't think is the case.)

[GuillaumeGomez]

I didn't take into account the callsite so I think it's better to just close this PR and continue to handle it directly in cg_gcc. Thanks for the review!

[rust-timer]

Finished benchmarking commit (17f23b92274eee4b3eba2dee1fff8264edf3d266): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.1%, 0.1%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 630.431s -> 632.095s (0.26%)
Artifact size: 315.96 MiB -> 315.98 MiB (0.01%)