[PATCH] D52131: [GISel][NFC]: Make MachineIRBuilder fully stateless

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added a comment.

In https://reviews.llvm.org/D52131#1236994, @qcolombet wrote:

> Hi Aditya,
>
> > this allows for the target/hook to be explicit about what they expect the builder to do.
>
> What do you mean by that?


When there are many kinds of MachineIRBuilders the question always comes out to what the default should be. Instead of picking a default (which most of the time targets won't change) - now various hooks/customlegalization methods will need to decide what kind of builder they need (Say CSE vs ConstantFolding vs Basic). With this, we only pass in the State, and the method/function will explicitly pick a builder.
Additionally I ran into a bug for the following code sequence.

  bool legalizeCustom(MachineIRBuilder &B) {
    ConstantFoldingBuilder CB(B.getState());
    CB.setInsertionPoint(SomeMI); // Sets the insertion point only for the CB's copy of the State/insertion point.
    if (someCondition)
       return someLegalizerHelper(B); // Pass B to a helper - but B's copy of the state has still no MBB/Insertion point set - only CB's copy was updated.
    else
       // Build using CB.
  }

In the above code someLegalizerHelper had assumed that the insertion point will always be set before it was called - but in this case it was not.
Instead the following code sequence would not have the above bug.

  bool legalizeCustom(MachineIRBuilderState &State) {
    ConstantFoldingBuilder CB(State);
    CB.setInsertionPoint(SomeMI);  // Changing the state that was passed in.
    if (someCondition)
      return someLegalizerHelper(State); // Insertion point is still reflected in State.
    // Build with CB.
  }



> Put differently, is there any example of that in that patch?
> 
> Cheers,
> -Quentin


Repository:
  rL LLVM

https://reviews.llvm.org/D52131