[llvm-commits] [PATCH 2/2] Convert VLDR/VSTR on demand for base update optimization

[Andrew Trick]

On Sep 18, 2012, at 10:30 AM, David Peixotto <dpeixott at codeaurora.org> wrote:
>>> +// Check to see if we can convert the instruction to a version that
>>> +// supports writeback to the base register.
>>> +// We can convert a VLDR/VSTR to VLDM/VSTM as long as the VLDR/VSTR
>>> +do // not have any pre-increment in the instruction (i.e. the imm in
>>> +the // instruction should be zero).
>>> +static bool canConvertToWriteBackInstr(const MachineInstr& MI) {
>>> +  unsigned Opcode = MI.getOpcode();
>>> +  switch (Opcode) {
>>> +  default: return false;
>>> +  case ARM::VLDRS:
>>> +  case ARM::VSTRS:
>>> +  case ARM::VLDRD:
>>> +  case ARM::VSTRD:{
>>> +    // Check to make sure we have an instruction of the form
>>> +    // VLDR <Dd>, [<Rn> {, #+-<imm>]
>>> +    if (MI.getNumOperands() < 3)
>>> +      return false;
>>> +    if (!MI.getOperand(1).isReg()) // Base address
>>> +      return false;
>> 
>> What conditions are these statements trying to check for? If they ever
> fire,
>> something has gone seriously wrong, as that would indicate a malformed
>> instruction.
> 
> This was just an attempt at defensive programming. I agree that they would
> indicate a malformed instruction. I'll remove the checks.

I would very much appreciate seeing those checks in the code. They can be asserts if Jim insists that they will hold, which is not obvious to anyone reading the ARM backend.

In general, we should be able to change instruction layouts or add implicit operands in the future without having arbitrary peephole opts silently generate incorrect code.

>>> +
>>> +  // Transfer any extra operands (e.g. implicit defs/kills)  for
>>> + (unsigned int i = 5; i < MI->getNumOperands(); ++i)
>>> +    MIB.addOperand(MI->getOperand(i));
>>> +
>> 
>> What cases have you encountered where a VLDR/VSTR instruction has any
>> additional operands that need to be copied?
> 
> I had taken this code from elsewhere in the file. Perhaps it is not
> necessary. The one time I can think of it having additional operations would
> be an implicit def of a D register when loading an S register. I'm not sure
> if those are modeled like that, so I'm happy to remove it if it looks
> unnecessary to you.

You definitely need to check for additional operands before doing this transformation. The question is do you copy them, bailout, or assert. To answer that question we need to know where else we explicitly verify the lack of additional operands. I'm not yet aware of any place.

-Andy