This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Optimize (add (shl x, c0), (shl y, c1))
AbandonedPublic

Authored by benshi001 on Sep 13 2021, 4:36 PM.

Details

Summary

Optimize (add (shl x, c0), (shl y, c1)) to

(SLLI (SHxADD y, x), c0) if applicable.

Diff Detail

Event Timeline

benshi001 created this revision.Sep 13 2021, 4:36 PM
benshi001 requested review of this revision.Sep 13 2021, 4:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2021, 4:36 PM

The reason I write both AddShlShl_1A and def AddShlShl_1B, is that add is not commutative in PatFrag.

jrtc27 added inline comments.Sep 13 2021, 4:42 PM
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
215

This should just be a templated class that takes the +/- 1/2/3 as an argument rather than copy-pasting the same thing 6 times. Though the number of patterns and special cases for all the bitmanip stuff seems to be getting rather crazy; is there an end in sight?

The reason I write both AddShlShl_1A and def AddShlShl_1B, is that add is not commutative in PatFrag.

You should write that as a comment in the code.

The reason I write both AddShlShl_1A and def AddShlShl_1B, is that add is not commutative in PatFrag.

You should write that as a comment in the code.

Actually it's worse than that. Tablegen does know the pattern is commutable and will call you predicate function twice. But one of the calls will be wrong. I hope it is guaranteed that the correct check always happens first. There is a feature to make this work. It involves setting "let PredicateCodeUsesOperands = 1;" on the PatFrag. Only AMDGPU uses it. You should look up the patch that added it.

benshi001 updated this revision to Diff 372395.Sep 13 2021, 9:10 PM

Using let PredicateCodeUsesOperands = 1 makes code more boring and complex. So I have resorted to using DAG2GAG selection which looks more clear.

benshi001 updated this revision to Diff 372396.Sep 13 2021, 9:19 PM

Using let PredicateCodeUsesOperands = 1 makes code more boring and complex. So I have resorted to using DAG2GAG selection which looks more clear.

I'm not sure what you mean by boring here. Can you share one of the PatFrags using that feature?

benshi001 updated this revision to Diff 372397.Sep 13 2021, 9:24 PM

Using let PredicateCodeUsesOperands = 1 makes code more boring and complex. So I have resorted to using DAG2GAG selection which looks more clear.

I'm not sure what you mean by boring here. Can you share one of the PatFrags using that feature?

I mean the similar code is copied 6 times which is boring, and it becomes more complex than I expected. and I think using DAG2DAG is more clear.

Using let PredicateCodeUsesOperands = 1 makes code more boring and complex. So I have resorted to using DAG2GAG selection which looks more clear.

I'm not sure what you mean by boring here. Can you share one of the PatFrags using that feature?

Using let PredicateCodeUsesOperands = 1 also needs implementing let GISelPredicateCode = [{...}], otherwise the TableGen will failed.

such as

def patfrags_test_pat : PatFrags<
  (ops node:$x, node:$y, node:$z),
  [ (xor (add node:$x, node:$y), node:$z),
    (xor (sub node:$x, node:$y), node:$z)
  ], [{ return foo(); }]> {
  let GISelPredicateCode = [{
    return doesComplexCheck(MI);
  }];

  let PredicateCodeUsesOperands = 1;
}

That make the code becomes more redundant and complex.

Using let PredicateCodeUsesOperands = 1 makes code more boring and complex. So I have resorted to using DAG2GAG selection which looks more clear.

I'm not sure what you mean by boring here. Can you share one of the PatFrags using that feature?

I mean the similar code is copied 6 times which is boring, and it becomes more complex than I expected. and I think using DAG2DAG is more clear.

The PredicateCodeUsesOperands feature should only require 3 PatFrags that look like this, but I can't get it to compile.

let PredicateCodeUsesOperands = 1 in
def AddShlShl_1A : PatFrag<(ops node:$A, node:$B, node:$C, node:$D),
                           (add (shl node:$A, node:$B),
                                (shl node:$C, node:$D)), [{
  SDValue N0 = Operands[0], N1 = Operands[1];
  if (!N0.hasOneUse() || !N1.hasOneUse())
    return false;
  auto *N0C = cast<ConstantSDNode>(N0.getOperand(1));
  auto *N1C = cast<ConstantSDNode>(N1.getOperand(1));
  uint64_t C0 = N0C->getZExtValue(), C1 = N1C->getZExtValue();
  return C0 == C1 + 1;                                                                                                                                                       
}]>;
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
539 ↗(On Diff #372397)

vevtor->vector

540 ↗(On Diff #372397)

The VT.getSizeInBits() > Subtarget->getXLen() should never fail. If the VT is scalar it must be XLenVT.

584 ↗(On Diff #372397)

There's already a DL in the top of this function.

benshi001 updated this revision to Diff 372399.Sep 13 2021, 9:51 PM
benshi001 edited the summary of this revision. (Show Details)
benshi001 marked 3 inline comments as done.
benshi001 updated this revision to Diff 372646.Sep 15 2021, 1:06 AM
benshi001 updated this revision to Diff 372702.Sep 15 2021, 7:25 AM

I'm inclined to prefer D108916 over this.

benshi001 abandoned this revision.Sep 18 2021, 5:04 PM