[Patch] [Thumbv8] Fix tje default value of BLXOperandIndex of isV8EligibleForIT

[Weiming Zhao]

Hi Artyom,

Thanks for reviewing the patch and explaining the reason of BLXOperandIndex
being 2 or 0.
I updated the patch per your suggestion. I also fixed the test case of
r199127.

The patch is committed as r199928.

Thanks again,
Weiming

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation


-----Original Message-----
From: Artyom Skrobov [mailto:Artyom.Skrobov at arm.com] 
Sent: Thursday, January 23, 2014 9:43 AM
To: weimingz at codeaurora.org
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [Patch] [Thumbv8] Fix tje default value of BLXOperandIndex of
isV8EligibleForIT

> Function isV8EligibleForIT(InstrType *Instr, int BLXOperandIndex = 0) 
> has
2
> call sites:
>
> ARMBaseInstrInfo.cpp:      return isV8EligibleForIT(MI);  è this one
should
> pass the 2nd parameter with value 2 (just like the other call)
>
> AsmParser/ARMAsmParser.cpp:          !isV8EligibleForIT(&Inst, 2))
>
>
> Below is an example of MCInst for “blx r8”:
>
> blx r8:
> @ <MCInst #2747 tBLXr
> @ <MCOperand Imm:14>
> @ <MCOperand Reg:0>
> @ <MCOperand Reg:74> ==> this is the operand to be checked
>
> Since there are only 2 call sites, this patch changes the default 
> value of BLXOperandIndex to 2 instead of 0.

Weiming, the original reason why BLXOperandIndex was introduced was because
the Rm for BLX was passed as operand #0 in MachineInstr, but as operand #2
in MCInst.

I don't know why this had been like that, or what has changed since then,
but you're right, it's now operand #2 in both cases, meaning that this
argument to isV8EligibleForIT is no longer needed at all:

Index: lib/Target/ARM/ARMFeatures.h
===================================================================
--- lib/Target/ARM/ARMFeatures.h	(revision 199881)
+++ lib/Target/ARM/ARMFeatures.h	(working copy)
@@ -19,7 +19,7 @@
 namespace llvm {
 
 template<typename InstrType> // could be MachineInstr or MCInst -inline
bool isV8EligibleForIT(InstrType *Instr, int BLXOperandIndex = 0) {
+inline bool isV8EligibleForIT(InstrType *Instr) {
   switch (Instr->getOpcode()) {
   default:
     return false;
@@ -70,14 +70,13 @@
     return true;
 // there are some "conditionally deprecated" opcodes
   case ARM::tADDspr:
+  case ARM::tBLXr:
     return Instr->getOperand(2).getReg() != ARM::PC;
   // ADD PC, SP and BLX PC were always unpredictable,
   // now on top of it they're deprecated
   case ARM::tADDrSP:
   case ARM::tBX:
     return Instr->getOperand(0).getReg() != ARM::PC;
-  case ARM::tBLXr:
-    return Instr->getOperand(BLXOperandIndex).getReg() != ARM::PC;
   case ARM::tADDhirr:
     return Instr->getOperand(0).getReg() != ARM::PC &&
            Instr->getOperand(2).getReg() != ARM::PC;
Index: lib/Target/ARM/AsmParser/ARMAsmParser.cpp
===================================================================
--- lib/Target/ARM/AsmParser/ARMAsmParser.cpp	(revision 199881)
+++ lib/Target/ARM/AsmParser/ARMAsmParser.cpp	(working copy)
@@ -7956,7 +7956,7 @@
 
       // Only after the instruction is fully processed, we can validate it
       if (wasInITBlock && hasV8Ops() && isThumb() &&
-          !isV8EligibleForIT(&Inst, 2)) {
+          !isV8EligibleForIT(&Inst)) {
         Warning(IDLoc, "deprecated instruction in IT block");
       }
     }

This change, together with your original test case, LGTM to commit.


On a related note, while reviewing the patch you've committed as r199127,
I've noticed that your added test case uses " | FileCheck %s" without
actually checking for anything in the output (other than that it's
successfully generated).

I suggest deleting the " | FileCheck %s" bit from the "RUN:" line, as well
as deleting the "CHECK-LABEL:" line.