LLVM Discussion Forums

materializeConversion for 1:1 conversion

Hi @River707, I think I’m hitting the following todo in dialect conversion/type converter:

      // FIXME(riverriddle) This should check that the result type and operand
      // type are the same, otherwise it should force a conversion to be
      // materialized.

I’ve got a variadic type that I am converting and it does the right thing if going from 1->0 or 1:N>1, but as the note above says, materializeConversion does not get called for N=1, and I do need to insert a cast in that case too. I spent a few minutes looking at how to resolve this FIXME but it wasn’t obvious (it seems like there is a lot of code written for various materializeConversion() calls that would be problematic if we started calling it for the 1:1 case). Do you have any insights?

Thanks!

I prototyped this in https://reviews.llvm.org/D73702 because I thought it was necessary for function signature conversion. It was not in the end, so I let it sit there. If this looks useful to use, we can clean up and push.

That does look like it would fix the issue I’m seeing. For the moment, in the work I’m doing, I’ve hoisted the signature conversion to a separate pass to work around this, but we’ve bumped up against this limitation a few times (and keep forgetting/getting surprised at it).

There’s also Extending type conversion infrastructure, which relates to one of @River707’s comments about wanting to generalize how these are extended. It isn’t clear to me whether we want to land your change first or do that refactor? (I haven’t started work on the refactor, so there is no loss on my side to landing what you have and then adapting, but refactoring first may reduce API churn).

Has there been any movement on this? We are bumping against this as well.

No, I’ve not had time to get back to this. It might be worth just finishing @ftynse’s patch to get this functionality correct.

https://reviews.llvm.org/D79729 provides similar functionality to my original change. I rewrote it mostly from scratch because TypeConverter changed in the meantime. If this fits the bill for you both, I can clean it up and send for review.

lgtm in principle. thanks!

FYI, this has landed today.