[PATCH] D42287: [GlobalISel][X86] Fixing failures after https://reviews.llvm.org/D37775

Quentin Colombet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 19 14:40:15 PST 2018


qcolombet added a comment.

> Should I submit autogenerated tests that are not FP/G_COPY/G_TRUNC related, but that still need to be updated for https://reviews.llvm.org/D37775 as a separate patch? Should I have a review for it or can I just submit?

I would just push the autogenerate checks first directly (no review needed).
That way, the tests will only have the FP/COPY/ANYEXT changes + their impact on autogenerated checks.



================
Comment at: lib/Target/X86/X86InstructionSelector.cpp:686
   if (!SrcRC)
     return false;
 
----------------
aivchenk wrote:
> qcolombet wrote:
> > Should we move these ![...]RC checks up?
> I'm not sure, to be honest. Added lines should be before
> 
>  if (DstRB.getID() != X86::GPRRegBankID)
>     return false;
> 
> Cause otherwise we will return in that check. At the same time those checks that you mention are after X86::GPRRegBankID one, so that move will change the logic. And I was trying to avoid changing the logic as I'm not that familiar with that code yet
I meant only the part that checks that the RegisterClasses are not nullptr.
If they are nullptr, the checks you're adding won't match anyway :).


================
Comment at: test/CodeGen/X86/GlobalISel/callingconv.ll:329
+; X86_64-NEXT:    .cfi_offset %rbx, -16
+; X86_64-NEXT:    movzbl (%rdi), %ebx
+; X86_64-NEXT:    movl %ebx, %edi
----------------
aivchenk wrote:
> qcolombet wrote:
> > Do you know why this got better all of the sudden?
> No, this change is not connected with my patch
Ok, hopefully this will disappear when you rebase your patch after pushing the autogenerated checks.


https://reviews.llvm.org/D42287





More information about the llvm-commits mailing list