Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Souper fails to turn multiplication by a power of two into a shift? #865

Closed
fitzgen opened this issue Jan 27, 2023 · 3 comments
Closed

Souper fails to turn multiplication by a power of two into a shift? #865

fitzgen opened this issue Jan 27, 2023 · 3 comments

Comments

@fitzgen
Copy link
Contributor

fitzgen commented Jan 27, 2023

Wanted to double check that my souper-check invocations were correct before running a large batch and was puzzled by the following failure to infer an RHS:

$ cat ~/scratch/foo.souper 
%1:i32 = var
%2:i32 = mul %1, 4:i32
infer %2

$ ~/souper/obj/souper-check --infer-rhs -souper-enumerative-synthesis-max-instructions=3 ~/scratch/foo.souper 
; Failed to infer RHS

I would expect it to synthesize %2:i32 = shl %1, 2:i32.

I tried all the different options to --cost-kind and get the same result.

Then I wondered if my souper-check was just broken, but it can successfully synthesize some right-hand sides where it needs to come up with a constant:

$ cat ~/scratch/bar.souper
%1:i32 = var
%2:i32 = mul %1, 2:i32
%3:i32 = mul %2, 3:i32
infer %3

$ ~/souper/obj/souper-check --infer-rhs -souper-enumerative-synthesis-max-instructions=3 ~/scratch/bar.souper 
; RHS inferred successfully
%3:i32 = mul 6:i32, %1
result %3

Any idea what's going on here? Am I missing some critical flag or something?

@regehr
Copy link
Collaborator

regehr commented Jan 27, 2023

hi Nick, cost models have been a major problem for Souper since day 1.

here's the current cost function from Inst.cpp:

int Inst::getCost(Inst::Kind K) {
  switch (K) {
    case Var:
    case Const:
    case UntypedConst:
    case SAddO:
    case UAddO:
    case SSubO:
    case USubO:
    case SMulO:
    case UMulO:
      return 0;
    case BitReverse:
    case BSwap:
    case CtPop:
    case Cttz:
    case Ctlz:
    case SDiv:
    case UDiv:
    case SRem:
    case URem:
      return 5;
    case FShl:
    case FShr:
    case SAddSat:
    case UAddSat:
    case SSubSat:
    case USubSat:
    case Select:
      return 3;
    default:
      return 1;
  }
}

there are three things we can do. first, we can tweak this, for example by giving mul cost 2, which should cause the result you want to be produced. second, you can hack your local souper (and I suggest you try that now, and see if it does what I said). third, we could make it possible to provide souper with a cost model externally. I've tweaked this stupid cost model many times and there's no good answer even within LLVM, and of course you're sort of misusing souper for non-LLVM purposes, so...

@regehr
Copy link
Collaborator

regehr commented Jan 27, 2023

to give a bit more history, I've asked a number of smart and experienced LLVM people what we should do for a cost model over the years. everyone has one or more ideas, and none of them have ended up being satisfactory. so probably for now just do whatever you want in this function.

of course a better cost model would (optionally) look at multiple instructions instead of just one. but that way leads to madness.

@fitzgen
Copy link
Contributor Author

fitzgen commented Jan 27, 2023

Thanks for the reply. Yeah, this definitely is a rabbit hole one could keep following.

Hacked up that method and I'm getting what I expect now. Thanks!

@fitzgen fitzgen closed this as completed Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants