[llvm-commits] [llvm] r140134 - in /llvm/trunk: include/llvm/MC/MCInstrDesc.h lib/CodeGen/SelectionDAG/InstrEmitter.cpp lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp lib/Target/ARM/ARMISelLowering.cpp lib/Target/ARM/ARMInstrInfo.td lib/Target/ARM/ARMInstrThumb2.td test/CodeGen/ARM/2011-09-19-cpsr.ll utils/TableGen/CodeGenInstruction.cpp utils/TableGen/CodeGenInstruction.h utils/TableGen/InstrInfoEmitter.cpp

[Andrew Trick]

On Sep 20, 2011, at 10:27 AM, Evan Cheng wrote:

> 
> On Sep 20, 2011, at 9:07 AM, Andrew Trick wrote:
> 
>> 
>> On Sep 20, 2011, at 8:49 AM, Evan Cheng wrote:
>> 
>>> 
>>> 
>>> On Sep 19, 2011, at 10:36 PM, Andrew Trick <atrick at apple.com> wrote:
>>> 
>>>> On Sep 19, 2011, at 9:55 PM, Evan Cheng wrote:
>>>> 
>>>>> Thanks! Just curious, could you have fixed the bug by adding hasPostIselHook = 1 to SUBS?
>>>>> 
>>>>> Evan
>>>>> 
>>>> 
>>>> t2SUBS was already under hasPostIselHook (T2I_bin_s_irs). Consequently, when its CPSR result was dead, the implicit def would be removed--that's wrong for any opcode that always sets the 's' bit. Hard-coding certain opcodes would be  a sufficient fix, but I wanted some form
>>> 
>>> 
>>> I'm missing something obvious then. If the cpsr def isn't used, why can't codegen remove the implicit def and the optional def? The net effect is to change it to the non-S variant. 
>> 
>> There's no non-S variant of the opcode. I'm not 100% sure why we can't convert the opcode to adds/subs.w later.
> 
> What do you mean? There is t2SUB. t2SUBS and t2SUB are identical opcodes if the CPSR def is dead. The reason we can't just isel to t2SUB is due to tablegen limitation (related to physical register def).

Great. That's what I wasn't 100% sure about. Let's remove the t2SUBS opcode completely.

> Is the bug due to Thum2SizeReduction changing it to tSUB? Perhaps the fix is for the isel hook to change the opcode from t2SUBS to t2SUB if the CPSR def is dead.


Looking at it that way, the "bug" is that ARM::t2SUBS is strangely always encoded as subs!
 
Changing the opcode in the hook would be superior to what I did. I wasn't sure that would be ok. Although it's still horribly confusing compared to removing the t2ADDS/SUBS opcodes, even if we need custom lowering to set CPSR (due to the tblgen limitation you mentioned?)

-Andy