Fix failed test

Didn't notice some test were failed.

Thanks !

Add comment

Thanks @lattner @bzcheeseman ! I'll land this patch after making sure everything runs smoothly.

fix typo

That's expected. I'll open an issue for it later this day.

@myhsu this file has dependency on MCFixedLenDisassembler.h, but I'm not quite sure whether we should take care of it, given that I did not encounter any failure during building and testing. What's your opinion ?

apply clang-format

update diff trying to fix buildbot failure

update diff

• define our own castFailed in the hope of fixing the build failure of clang.
• add static_assert for type check

It seems that people are not always using a plain pointer in PointerUnion. Sometimes they seem to use a custom smart pointer ?

• public inheritance for ConstStrippingForwardingCast

Address feedback

• Add test case for const
• Add some comments.

I used arc incorrectly. Re-update the diff.

Thanks for feedback ;)

Implement CastInfoPointerUnionImpl

I can only come up with this :

Didn't notice the build failure. Remove the CastInfoPointerUnionImpl and revert
back to the original implementation.

In D125609#3513978, @0x59616e wrote:

Given that I'm still young to C++ and llvm, every suggestion, even the
one you feel it's trivial, are very welcomed ;)

This diff comes with the following updates:

• Define PointerUnion::is and ::get in terms of isa<T> and cast<T>.
• Add FIXME comment for is(), get() and dyn_cast().

Given that I'm still young to C++ and llvm, every suggestion, even the
one you feel it's trivial, are very welcomed ;)

Thanks ! Appreciate your help ;)

The comment is updated :
1284ce917b5a

fix the include guard

I've commited a diff in the hope of fixing this problem:

change the header guard

rebase

In D120958#3490184, @0x59616e wrote:

In D120958#3487811, @foad wrote:

For the record, to get this to build with my downstream target, I had to add:

MyInsnType MyInsnType::operator&(const uint64_t &RHS);
bool MyInsnType::operator!=(const int &RHS);

Thanks for your information. I'll update the comment.

In D120960#3488691, @myhsu wrote:

In D120960#3486430, @0x59616e wrote:

In D120960#3484760, @myhsu wrote:

just want to double check: all of the previously-XFAIL tests are now passing, right?

Yes. I don't see any 'unexpectedly passed' test.

This is a good news and proofs that the new variable-length decoder infrastructure works at scale :-)
(at least at a certain scale given that we still more coverage on our disassembler tests)

In D120958#3487811, @foad wrote:

For the record, to get this to build with my downstream target, I had to add:

MyInsnType MyInsnType::operator&(const uint64_t &RHS);
bool MyInsnType::operator!=(const int &RHS);

In D120958#3484756, @myhsu wrote:

lgtm. Thanks for the patch!

In D120960#3484760, @myhsu wrote:

just want to double check: all of the previously-XFAIL tests are now passing, right?

address feedback:

• use llvm::alignTo
• construct APInt directly with constructor instead of assignment copy constructor

ping. Any more suggestions ?

continue addressing formating issues

Revert the TODO suggested my myhsu

address format issue

rebase

rebase

I could not remember whether I've modified this patch recently.

I know what is going on.

I've noticed the clang-format problem. The weird thing is, It happened after I rename this file, before I change anything in it. That makes me confusing. It should have not happened because I only change the file name.

Rename the class name.

rebase

Should we rename after this patch ? or before this patch ?

In D120958#3419921, @myhsu wrote:

In D120958#3416426, @0x59616e wrote:

In D120958#3415717, @myhsu wrote:

The logics look cleaner now.
Now a bigger question is: Should we still calling it FixedLenDecoderEmitter?

What about DecoderEmitter.cpp ?

Sounds good to me, but I would suggest you to send a RFC to the forum asking for consensus.
Especially to see if there is any comment from existing FixedLenDecoder users (e.g. AArch64 and ARM). Also, in the RFC, please briefly explain why you want to build this feature on top of FixedLenDecoder rather than writing a separate disassembler.

address feedback

address feedback

In D120958#3415717, @myhsu wrote:

The logics look cleaner now.
Now a bigger question is: Should we still calling it FixedLenDecoderEmitter?

move test into another file

address some feedbacks

formatting

Remove tablegen variable isVarLenInst. It's useless

fix a minor problem

rebase

address feedback

In D120958#3383624, @myhsu wrote:

In D120958#3382237, @0x59616e wrote:

We can traverse the Segments of VarLenInst and use CGIOperandList::OperandInfo::ParseOperandName to get the operand number, and then add that Segments into the correspoding OpInfo according to the operand number.

For example, if we find "dst.reg" and use CGIOperandList::OperandInfo::ParseOperandName and know "Oh, this is the third operand", then we can add this into the third OpInfo.

In this way we can avoid overload BitsInit completely.

This approach sounds reasonable to me

In D120958#3380177, @myhsu wrote:

Thank you for the patch.
A high level question: It seems like generating a decoder requires two phases -- First, generates a list of OperandInfo, which is currently done by populateInstruction, then emit the real (C++) code according to these OperandInfo instances. My question is, can we use our own way -- possibly putting in a separate file -- to generate these OperandInfo before supplying them to the second phase, instead of trying to piggyback everything into the existing FixedLenDecoder framework?
Because, to be honest, I'm not a big fan of overloading the BitsInit (to carry operand info). Using a BitsInit might fit well for fixed-length instructions but I feel like there are more elegant ways to handle var-length instructions. For instance, using CGIOperandList::OperandInfo::ParseOperandName to parse suboperands rather than traversing every single in/out operands in the original instruction definition.

Fix a minor problem in the test