[PATCH] D83384: [GlobalISel][InlineAsm] Fix buildCopy for inputs

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:240
 
+static bool buildAnyextOrCopy(Register Dst, Register Src,
+                              MachineIRBuilder &MIRBuilder) {
----------------
Petar.Avramovic wrote:
> paquette wrote:
> > Would `MachineIRBuilder::buildExtOrTrunc` work here?
> > 
> > If not, maybe it would make sense to move this to `MachineIRBuilder` for the sake of consistency?
> The destination is vreg with reg class and source is generic vreg with LLT, it would not work for anyext since it requires both source and dest to be generic virtual registers. Here we anyext to new generic vreg with same size as Dst and then copy to Dst.
> MachineIRBuilder specializes for generic vregs so should it be something like: "buildExtOrTruncToVRegWithRegClass" ? Should I also cover vectors? I don't know if there is a way to know LLT of vector type that would fit into reg class.
> 
I see.

Considering that difference, I think this is fine. We can refactor later if it turns out to be a good idea.

As for vectors, I think that would be better in a follow-up.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:254
+  if (DstSize > SrcSize) {
+    if (!MRI->getType(Src).isValid() || !MRI->getType(Src).isScalar()) {
+      LLVM_DEBUG(dbgs() << "Can't extend input to size of destination"
----------------
Can we check `MRI->getType(Src).isValid()` before here? I think we always want to check this, right?

e.g.

```
auto SrcTy = MRI->getType(Src);
if (!SrcTy.isValid()) {
    LLVM_DEBUG(dbgs() << "Source type for copy is not valid\n");
    return false;
}

if (DstSize < SrcSize) {
  ...
}

// Attempt to anyext small scalar sources.
if (DstSize > SrcSize) {
  ...
}

...

```


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

https://reviews.llvm.org/D83384