ping

ping

This was reverted in order to revert D36084. Investigating the problem with D36084

Two windows bots failed select-inc.mir which should have been unaffected by the change. Investigating

Removed the reference to zero-extending APInt's from the summary since this isn't in the patch anymore. IntImmLeaf with a predicate that uses Imm->getZExtValue() is equivalent so I didn't see a need to make uint64_t predicates separately.

Rebase

Fixed the priority sort-order nits:

• Returns false if both sides are invalid instead of aborting.
• Avoid checking size-in-bits on separate code-paths and handle it in one place.
• Also order pointers and pointer address spaces since there's no longer an unreachable for them to fall into.

Fix the issues with the changes to the C++ implementation of G_INTTOPTR and G_BITCAST.

That makes sense to me, I'll switch it over to sign-extend.

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

Fix == TargetOpcode::G_CONSTANT nit.

In D36094#828809, @fhahn wrote:

I'll commit D36151 after this one goes in, so we see if this change fixes the buildbot

It's been pointed out to me that one of the bots (http://green.lab.llvm.org/green/job/Compiler_Verifiers_GlobalISEL/) is red because of the bug this fixes. That being the case, I'm going to commit this and we can post-commit review it.

In D36151#827515, @fhahn wrote:

Yes, D36094 fixes the problem too! Here's what's going on in the test case, without any of the 2 patches:

Limiting memory operands to instructions involving memory is a change I want to make but before we do that, could you elaborate a bit on the spurious memory operand you're seeing? In particular, was that memory operand present on any of the instructions covered by the match? I'm wondering if you've hit the bug that will be fixed by https://reviews.llvm.org/D36094.

Fix the previous update so that it contains both the original patch and the correction.

The C++ uses ZExt but TableGen uses SExt. I haven't decided how to fix this one yet.

In D36084#825975, @dsanders wrote:

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

I had a testcase for this one but it seems to have gone missing. I'll update with one shortly

Added assertion.
Added support for isImm() case.
Switched to negative number on the i32 test case.

• Fixed the issue preventing tablegen from matching either case and killed the C++ that made the test pass.
• Fixed a priority bug that was revealed after that fix. G_CONSTANT instructions need to be less important than specific constants.
  • I'm thinking that I should replace ConstantIntOperandMatcher with an appropriate instruction/opcode/literal-int matcher combination. I see no good reason for it to be special at this point. I'll leave that for another patch though.

LGTM with a better commit message.

Could you run clang-format on this patch? There's a few indentation and style nits in this patch. There's also a few in the neighbouring code which we should deal with separately

Add missing break
Removed State.MIs.resize(1). The first GIM_RecordInsn will do it.
Moved CurrentLabelID inside MatchTable

In D35516#819039, @aditya_nandakumar wrote:

@dsanders - the main goal was to avoid the compiler warning of having to create a zero sized array.

I'm not sure I understand why this change is needed. I'd expect the original version of buildInstr<unsigned>, to expand to:

MachineInstrBuilder buildInstr(unsigned Opc, unsigned &&Ty) {
  auto MIB = buildInstr(Opc).addDef(getDestFromArg(Ty));
  unsigned It[] = {};
  for (const auto &i : It)
    MIB.addUse(i);
  return MIB;
}

which should then optimize to:

MachineInstrBuilder buildInstr(unsigned Opc, unsigned &&Ty) {
  auto MIB = buildInstr(Opc).addDef(getDestFromArg(Ty));
  return MIB;
}

which appears to be the same thing the new version expands and optimizes to.

Hi Aditya,

In D21465#814320, @KoviRobi wrote:

Hello,

Are there plans to merge this in? I have updated the patch onto 0d7672e (attached), if it is helpful, though it was mostly just s/target-abi[ =]/mabi=/ on the files which failed the tests (and then ran the tests again and nothing failed).

Thanks,
Robert

D21465-on-0d7672e.patch168 KB

Download

Thanks. I'll fix these for the commit.

Rebased.
Fixed accidental usage of C++14
Fixed GIM_Reject always rejecting instead of exiting the current GIM_Try block when appropriate.
Remove whitespace around generated comments.

Rebase before re-commit

In D33764#799474, @dsanders wrote:

In D33764#799357, @chapuni wrote:

I see. STATISTIC value, like NumPatternEmitted, is disabled with noop in -Asserts. See ADT/Statistic.h.

I fixed that in the previous patch but it looks like I missed a bit in this one. I'll fix this.