Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aarch64 global isel backend miscompiles funnel shift with undef amount #57256

Open
regehr opened this issue Aug 19, 2022 · 8 comments
Open

aarch64 global isel backend miscompiles funnel shift with undef amount #57256

regehr opened this issue Aug 19, 2022 · 8 comments

Comments

@regehr
Copy link
Contributor

regehr commented Aug 19, 2022

here's one from our unit tests:

declare i32 @llvm.fshl.i32(i32, i32, i32) #0

define i32 @fshl_scalar_undef_shamt() {
  %1 = call i32 @llvm.fshl.i32(i32 1, i32 2, i32 undef)
  ret i32 %1
}

the global isel backend returns an arbitrary 32-bit value: https://gcc.godbolt.org/z/Yvbx9P7K4

but that's not OK. this is the set of permissible return values:

1
2
4
8
16
32
64
128
256
512
1024
2048
4096
8192
16384
32768
65536
131072
262144
524288
1048576
2097152
4194304
8388608
16777216
33554432
67108864
134217728
268435456
536870912
1073741824
2147483649

cc @aemerson @ornata @nunoplopes @ryan-berger @nbushehri @zhengyang92 @efriedma-quic

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 19, 2022

@llvm/issue-subscribers-backend-aarch64

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 19, 2022

@llvm/issue-subscribers-backend-aarch64

@regehr
Copy link
Contributor Author

regehr commented Aug 24, 2022

here are a couple more that fit the same pattern, probably more places where freeze is needed

https://gcc.godbolt.org/z/7zP4dKTGo

both of these are straight out of our unit test suite, no fuzzing is occurring

@aemerson aemerson self-assigned this Sep 5, 2022
@ornata
Copy link

ornata commented Oct 1, 2022

https://reviews.llvm.org/D135020 fixes the urem_undef_lhs case

ornata pushed a commit that referenced this issue Oct 1, 2022
This fixes the `urem_undef_lhs` case in the following:

https://gcc.godbolt.org/z/Wo9x7o679

Also see #57256 for more related
bugs.

This is equivalent to the undef bits in `simplifyDivRem` in the DAGCombiner.

Differential Revision: https://reviews.llvm.org/D135020
@ornata
Copy link

ornata commented Oct 1, 2022

@ornata
Copy link

ornata commented Oct 1, 2022

Funnel shifts: https://reviews.llvm.org/D135022

I just matched the SelectionDAG behaviour here, which for scalars replaces the shift with the first or second operand. I'm not sure if that's the correct behaviour (since I can't seem to find the code that actually handles this), but it's more correct than not emitting anything.

ornata pushed a commit that referenced this issue Oct 1, 2022
SDAG does this, GISel doesn't.

See https://gcc.godbolt.org/z/sqjMx3Tfv

More context:
#57256

Differential Revision: https://reviews.llvm.org/D135021
@regehr
Copy link
Contributor Author

regehr commented Oct 10, 2022

current status is that I'm seeing this issue on funnel shift (@ornata has a patch in flight) and then also srem: https://gcc.godbolt.org/z/Td4qEaz43

@aemerson
Copy link
Contributor

Picking this back up, I think we need to address the concerns about the differences between scalar/vectors in https://reviews.llvm.org/D135022 and then implement fold for SREM as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants