[PATCH] D55988: WIP: RegBankSelect: Support some more complex part mappings

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/RegisterBankInfo.h:629
+  /// be inserted.
+  virtual unsigned getBreakDownCost(const ValueMapping &ValMapping) const {
+    return std::numeric_limits<unsigned>::max();
----------------
arsenm wrote:
> qcolombet wrote:
> > arsenm wrote:
> > > qcolombet wrote:
> > > > I feel we miss a register bank in that prototype (what you'd referenced as CurBank).
> > > I had this initially, but then discovered it will always be null. The array of banks is present in ValMapping
> > The banks we have in the value mapping are the one we want to use (destination), not the one the value originates from (source), right?
> > 
> > >  then discovered it will always be null. 
> > 
> > Are you just patching up the destination in your case (i.e., things that are not assigned to any regbank yet)?
> > 
> > I would have expected this information to be here and useful in cases like this:
> > orig: dest = op src
> > candidate mapping: dest.low = op.low src.low; dest.high = op.high src.high
> > Thus src has a regbank at this point, but dest doesn't.
> > 
> > Although I am not a fan of such approach, we could take the convention for now that curbank == null means we are patching a definition and curbank != null means we are patching a source.
> > E.g.,
> > 1. dest = op => dest.low = op.low ; dest.high = op.high
> > vs.
> > 2. = op src => = op.low src.low ; = op.high src.high
> > 
> > In #1 dest's regbank would be nullptr, in #2 src's regbank would be !nullptr.
> I based this off only seeing null, plus the above assert:
> 
>   assert((!IsSameNumOfValues || CurRegBank) && "We should not have to repair");
> 
That's not exactly what the intent of the assert was.
  // If MO does not have a register bank, we should have just been
  // able to set one unless we have to break the value down.
  assert((!IsSameNumOfValues || CurRegBank) && "We should not have to repair");

This should be refined such that `CurRegBank == nullptr` is allowed only if MO is a definition.
Essentially what this assert does is making sure that the `breakdown == 1` code path will have non-nullptr values.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55988/new/

https://reviews.llvm.org/D55988