[GlobalISel][AArch64] Two load instructions generated for single volatile load

[john-brawn-arm]

Bugzilla Link	36018
Resolution	FIXED
Resolved on	Jan 25, 2018 07:28
Version	trunk
OS	Linux
Blocks	#35152
CC	@aemerson,@dsandersllvm,@efriedma-quic,@zmodem,@qcolombet

Extended Description

For the following IR:

@​g = global i16 0, align 2
declare void @​bar(i32)

define hidden void @​foo() {
%1 = load volatile i16, i16* @​g, align 2
%2 = sext i16 %1 to i32
call void @​bar(i32 %2)
ret void
}

llc -mtriple=aarch64 -global-isel generates for the load followed by sext:

adrp x8, g
add x8, x8, :lo12:g
ldrh wzr, [x8]
ldrsh w0, [x8]

g is loaded twice, once with a zero-extending load which is discarded, and second with a sign-extending load. For a volatile load we should be generating only a single load instruction.

[john-brawn-arm]

assigned to @aemerson

[aemerson]

:( I'll take a look.

[aemerson]

We're matching the sext(load) into the ldrsh, however the tablegen imported matching rules only delete the G_SEXT instruction, leaving the original load. We're currently discussing the best way to fix this.

[dsandersllvm]

This is a variant of the known issues where sign-extending loads have to have a corresponding non-extending load and where something can modify the G_SEXT seperately from the G_LOAD and possible make the pair illegal.

The exact problem is different (in this case DCE can't eliminate the volatile load so it ends up with two volatile loads) but the root cause is the same: A sign-extending load is represented as two operations.

I'm currently in the process of rewriting the LegalizerInfo to make it possible to change the representation from:
%0 = G_LOAD:s16 :: load 16 from %addr
%1 = G_SEXT:s32 %0
into:
%1 = G_SEXTLOAD:s32 :: load 16 from %addr
which will prevent this particular case from occurring.

I think there's more variants of this though. Anything where there's a volatile load/ that isn't the top-level instruction ought to be taking steps to avoid duplicating the volatile load (it should require hasOneUse() to match and should delete the volatile load afterwards). I think we're ok for volatile stores since they don't have any results and therefore can't be nested inside something else.

[qcolombet]

I am slightly confused.
Didn't we say that we didn't want to have several load opcodes?
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171204/507184.html

[dsandersllvm]

I am slightly confused.
Didn't we say that we didn't want to have several load opcodes?
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171204/507184.html

We reconsidered that later in that thread after that commit received some objections about the original approach http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171204/507287.html

[qcolombet]

Ah right, thanks for reminding me that part of the thread!

[aemerson]

Committed a workaround until we fix the issue properly with a G_SEXTLOAD in r323371.

[efriedma-quic]

Needs to be cherry-picked to 6.0.

[zmodem]

Needs to be cherry-picked to 6.0.

Merged in r323434