This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] try to narrow shifted bswap-of-zext
ClosedPublic

Authored by spatel on Mar 21 2022, 11:28 AM.

Details

Summary

This is the IR counterpart to 370ebc9d9a573d6 which provided a bswap narrowing fix for issue #53867.

Here we can be more general (although I'm not sure yet what would happen for illegal types in codegen - too rare to worry about?):
https://alive2.llvm.org/ce/z/3-CPfo

This assumes that we have moved the shift after the bswap as proposed in D122010.

Diff Detail

Event Timeline

spatel created this revision.Mar 21 2022, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 11:28 AM
spatel requested review of this revision.Mar 21 2022, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 11:28 AM
RKSimon accepted this revision.Mar 21 2022, 3:07 PM

LGTM

This revision is now accepted and ready to land.Mar 21 2022, 3:07 PM
This revision was landed with ongoing or failed builds.Mar 22 2022, 5:29 AM
This revision was automatically updated to reflect the committed changes.

This patch causes a crash while building the Linux kernel for arm64. A simplifier C reproducer:

$ cat csio_lnode.i
struct csio_lnode {
  unsigned nport_id
};
csio_ln_vnp_read_cbfn_mbp_0;
csio_ln_vnp_read_cbfn_rsp_0() {
  struct csio_lnode *ln = csio_ln_vnp_read_cbfn_mbp_0;
  int nport_id;
  memcpy(&nport_id, csio_ln_vnp_read_cbfn_rsp_0, 3);
  ln->nport_id =
      __builtin_constant_p(0) ? nport_id << 24 | (nport_id & 83040) >> 8 : 0;
  ln->nport_id = ln->nport_id >> 8;
}

$ clang --target=aarch64-linux-gnu -O2 -c -o /dev/null csio_lnode.i
...
bswap must be an even number of bytes
  %3 = tail call i24 @llvm.bswap.i24(i24 %2)
in function csio_ln_vnp_read_cbfn_rsp_0
fatal error: error in backend: Broken function found, compilation aborted!
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: clang --target=aarch64-linux-gnu -O2 -c -o /dev/null csio_lnode.i
1.      <eof> parser at end of file
2.      Code generation
3.      Running pass 'Function Pass Manager' on module 'csio_lnode.i'.
4.      Running pass 'Module Verifier' on function '@csio_ln_vnp_read_cbfn_rsp_0'
...

I have reverted this in 4e0008dcbe9fce99b9727e8bbeb129efc7bf2d80 to hopefully keep our CI from going completely red. I am happy to test a new revision as needed.

I have reverted this in 4e0008dcbe9fce99b9727e8bbeb129efc7bf2d80 to hopefully keep our CI from going completely red. I am happy to test a new revision as needed.

Thanks for the revert! I missed a check on the source type size, so we tried to create a bogus bswap of i24. I'll add a regression test based on the provided source file and update the patch.

We (Linaro) also saw the same crash when building for Arm.

We (Linaro) also saw the same crash when building for Arm.

Thanks - I got an automated mail from your tester a few minutes ago. Please let me know if you see any problems with the updated version of the patch ( 0fcff69bcb3d ).

Oh good to know, I wasn't sure if we were sending reports externally.

Thanks for the fix, I'll keep you posted. (and you can always reply to the email if you have questions)

Oh and you might get some more mails as it runs through different configurations. Again I'll check if they're new or not.