This is an archive of the discontinued LLVM Phabricator instance.

Add disassembly for the conditioned move immediate instruction for the ARC backend
ClosedPublic

Authored by thomasjohns on Jul 7 2021, 9:50 AM.

Details

Summary

This adds and entry for MOVcc_ru6 (e.g. mov.eq %r0, 32) in ARCInstrInfo.td, its associated dependencies, and the ability to decode this instruction from an object file.

For example, when disassembling the object file derived from compiling:

int f(int x) {
  return __builtin_clz(x);
}

we used to have:

00000000 <f>:
       0: 2f 28 13 80   <unknown>
       4: ca 20 21 08   <unknown>
       8: e0 7f         j_s.d   [%blink]
       a: ce 20 e2 07   <unknown>

but now we have:

00000000 <f>:
       0: 2f 28 13 80   <unknown>
       4: ca 20 21 08   mov.eq  %r0, 32
       8: e0 7f         j_s.d   [%blink]
       a: ce 20 e2 07   <unknown>

Diff Detail

Event Timeline

thomasjohns created this revision.Jul 7 2021, 9:50 AM
thomasjohns requested review of this revision.Jul 7 2021, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 9:50 AM

Could you fix the issues reported by clang-tidy?

gyiu added a comment.Jul 7 2021, 1:04 PM

Do we also need a LIT testcase for this under llvm/test/CodeGen/ARC?

Could you fix the issues reported by clang-tidy?

Absolutely. Working on this now.

Do we also need a LIT testcase for this under llvm/test/CodeGen/ARC?

This change is only aiming to get the disassembly working (and thus the llvm/test/MC/Disassembler/ARC case). Our plan is to get the code-gen side of the CLZ intrinsic working in a follow up patch, and then we'll add the codegen LIT testcase there. Does this sound okay?

gyiu added a comment.Jul 7 2021, 1:20 PM

Do we also need a LIT testcase for this under llvm/test/CodeGen/ARC?

This change is only aiming to get the disassembly working (and thus the llvm/test/MC/Disassembler/ARC case). Our plan is to get the code-gen side of the CLZ intrinsic working in a follow up patch, and then we'll add the codegen LIT testcase there. Does this sound okay?

Sounds good to me.

Fix the straightforward clang-tidy lints.

marksl accepted this revision.Jul 8 2021, 9:12 AM

Your change looks good to me

This revision is now accepted and ready to land.Jul 8 2021, 9:12 AM
thomasjohns added inline comments.Jul 8 2021, 10:03 AM
llvm/lib/Target/ARC/Disassembler/ARCDisassembler.cpp
110

The recommendation for this lint is to move to decodeCCRU6Instruction. However, most backends seem to follow the Decode* naming convention and we do too for other Decode methods.

Changing all our methods to decode* is one option (PowerPC does this), though it will increase the diff size by a fair amount.

Another option is to add // NOLINTNEXTLINE(readability-identifier-naming) comments which the AMDGPU backend does.

I feel like other backends might have either ignored this or have submitted code before the clang-tidy pre-merge check was around.

thomasjohns added inline comments.Jul 8 2021, 10:14 AM
llvm/lib/Target/ARC/Disassembler/ARCDisassembler.cpp
14

For these, I need to do some research for how to workaround. With the way the buildkite bot checks this, it doesn't seem to pick up our compile_commands.json correctly or something like that.

This revision was landed with ongoing or failed builds.Jul 12 2021, 12:37 PM
This revision was automatically updated to reflect the committed changes.