This is an archive of the discontinued LLVM Phabricator instance.

[M68k] Add instruction selection support for zext with PCD addressing
ClosedPublic

Authored by ids1024 on Apr 23 2023, 10:00 PM.

Details

Summary

Instruction selection was failing when trying to zero extend a value loaded from a PC-relative address. This adds support for zero extension using the "program counter indirect with displacement" addressing mode. It also adds a test with code that was previously failing to compile.

This fixes a compile error in Rust's libcore.

Diff Detail

Event Timeline

ids1024 created this revision.Apr 23 2023, 10:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2023, 10:00 PM
ids1024 requested review of this revision.Apr 23 2023, 10:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2023, 10:00 PM

Thanks for your patch! I just gave it a try but I'm still running into this error when trying to build libcore:

In function: _ZN4core3num7dec2flt6lemire13compute_float17h5485431827d3a879E
error: could not compile `core`
LLVM ERROR: Cannot select: t13: i32 = mul t12, t36
  t12: i32,ch = CopyFromReg t0, Register:i32 %27
    t11: i32 = Register %27
  t36: i32,ch = load<(load (s8) from %ir.70, !alias.scope !40, !noalias !45), zext from i8> t0, t2, undef:i32
    t2: i32,ch = CopyFromReg t0, Register:i32 %30
      t1: i32 = Register %30
    t4: i32 = undef
In function: _ZN83_$LT$memchr..memmem..FindIter$u20$as$u20$core..iter..traits..iterator..Iterator$GT$4next17h404867d129058870E
error: could not compile `memchr`
rustc: /data/home/glaubitz/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:10477: void llvm::SelectionDAG::ReplaceAllUsesWith(llvm::SDNode*, llvm::SDNode*): Assertion `(!From->hasAnyUseOfValue(i) || From->getValueType(i) == To->getValueType(i)) && "Cannot use this version of ReplaceAllUsesWith!"' failed.
rustc exited with signal: 6 (SIGABRT) (core dumped)
error: could not compile `rustc-demangle`

Do we need to change something else?

Thanks for your patch! I just gave it a try but I'm still running into this error when trying to build libcore:

In particular, this patch fixes compilation of parse_long_mantissa. That error message doesn't seem to involve PC-relative addressing so it wouldn't be impacted by this patch. It may be unrelated or a different issue with zero extension.

I think there are other issues related to this. Even if zero-extension doesn't support PCD addressing, it presumably should be able to load the value and zero extend it with more instructions.

Actually, after too much debugging, it looks like that error is an issue with mul when targeting the 68020. You can work around that by changing setOperationAction(ISD::MUL, MVT::i32, Legal); to setOperationAction(ISD::MUL, MVT::i32, LibCall);, but after that I still have a build error LLVM ERROR: Cannot select: t42: ch = AtomicStore<(store unordered (s16) into %ir.30)> t40, t17, t38, src/mem/mod.rs:77:13.

So some more instruction selection fixes are needed for mul and AtomicStore.

Actually, after too much debugging, it looks like that error is an issue with mul when targeting the 68020. You can work around that by changing setOperationAction(ISD::MUL, MVT::i32, Legal); to setOperationAction(ISD::MUL, MVT::i32, LibCall);, but after that I still have a build error LLVM ERROR: Cannot select: t42: ch = AtomicStore<(store unordered (s16) into %ir.30)> t40, t17, t38, src/mem/mod.rs:77:13.

So some more instruction selection fixes are needed for mul and AtomicStore.

Thanks a lot for debugging this. I had suspected MUL to be one of the problems, but I still lack some understanding on how LLVM IR works.

Maybe Sheng and Min can comment on this change and the other necessary fixes.

myhsu accepted this revision.Apr 27 2023, 3:54 PM

LGTM Thanks!
Though could you update the patch description so that it's more descriptive? You can take a look at the commit message policy.

Actually, after too much debugging, it looks like that error is an issue with mul when targeting the 68020. You can work around that by changing setOperationAction(ISD::MUL, MVT::i32, Legal); to setOperationAction(ISD::MUL, MVT::i32, LibCall);, but after that I still have a build error LLVM ERROR: Cannot select: t42: ch = AtomicStore<(store unordered (s16) into %ir.30)> t40, t17, t38, src/mem/mod.rs:77:13.

So some more instruction selection fixes are needed for mul and AtomicStore.

I wouldn't lower 32-bit MUL to libcall when targeting >68020 since it already has native instructions for that size. For the AtomicStore issue it should be an easy fix: lower it to libcall same as AtomicLoad.

This revision is now accepted and ready to land.Apr 27 2023, 3:54 PM

I wouldn't lower 32-bit MUL to libcall when targeting >68020 since it already has native instructions for that size.

Yeah, definitely a workaround rather than a correct fix. But the fact it works with libcall and not legal suggests something is needed in instruction selection to properly generate the multiply instruction here. If I understand correctly.

ids1024 edited the summary of this revision. (Show Details)Apr 27 2023, 4:12 PM

Hm, that compile error seems to go away if I set ATOMIC_STORE to use Expand, but not LibCall? I'm not sure exactly how these are converted to library calls or "expanded". With that and a couple other hacks, Rust's libcore seems to compile.

@myhsu Could you land this change? I think @ids1024 doesn't have commit access.

This revision was landed with ongoing or failed builds.Apr 29 2023, 4:31 PM
This revision was automatically updated to reflect the committed changes.