[PATCH] D36990: [GISel]: Translate phi into generic G_PHI instruction

Quentin Colombet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 23 10:20:02 PDT 2017


qcolombet added inline comments.


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:423
   // or already have some operands assigned to banks.
-  if (!isPreISelGenericOpcode(Opc)) {
+  if (!isPreISelGenericOpcode(Opc) || Opc == TargetOpcode::G_PHI) {
     const RegisterBankInfo::InstructionMapping &Mapping =
----------------
aditya_nandakumar wrote:
> qcolombet wrote:
> > We should need to do that.
> > Could you describe what happen if you don't?
> When it returns the best mapping, it creates for NumOperands (say with 2 PHI values - it's 5). Then it fails the verification of the best mapping.
> assert(NumOperands == (isCopyLike(MI) ? 1 : MI.getNumOperands()) && "NumOperands must match, see constructor");
> I made the change so G_PHI would have been treated as it were a PHI.
Ah right, we want to use the generic getInstrMappingImpl, but now G_PHI falls into the isPreISelGenericOpcode category whereas PHI did not.

That being said, all the targets will have the exact same problem therefore, long term, we probably want to have some kind of generic method for this check. Anyhow, that's not really a problem for now and actually we could just skip the check and that would work (as in always calling getInstrMappingImpl and check if the Mapping is valid), though it would a waste of compile time.

Anyway, the current change makes sense and I'd expect you'd need to do the same for x86, amdgpu, and arm.


================
Comment at: test/Verifier/test_g_phi.mir:1
+#RUN: not llc -mtriple=aarch64-unknown-unknown -o - -global-isel -run-pass=legalizer -verify-machineinstrs %s
+--- |
----------------
Could you add a FileCheck to check we issue the expected error?


================
Comment at: test/Verifier/test_g_phi.mir:28
+---
+name:            test_phi
+legalized:       true
----------------
Could you add a comment on what this test is checking?


https://reviews.llvm.org/D36990





More information about the llvm-commits mailing list