[PATCH] D54409: PowerPC/SPE: Fix load/store handling for SPE

[Kei Thomsen via Phabricator via llvm-commits]

kthomsen added a comment.

I found an even better way to check for the Unsigned (thank you for the hint) 8bit with alignment 8 offset. It has now been integrated into the SelectAddressEVXRegReg() function. Therefore the calling function SelectAddressRegReg() doesn't need to deal with any EVx instructions specials, it is all done in the EVXRegReg function.
Explanation for the SelectAddressEVXRegReg function: If it can find a MVT::f64 (there can only be maximal one MVT::f64) , the offset used here will be checked if it is known now and if the offset is 0..255, with an alignment of 8. Then a false is returned and the calling SelectAddressRegReg() will continue with the isIntS16Immediate(), which will be true as well (if it fits into 8 bit unsigned, it will fit into 16 bit signed as well).

  --- PPCISelLowering.orig.cpp    2019-07-01 09:08:11.444438400 +0200
  +++ PPCISelLowering.cpp 2019-07-01 08:45:16.911244700 +0200
  @@ -2227,9 +2227,27 @@ bool llvm::isIntS16Immediate(SDValue Op,
     return isIntS16Immediate(Op.getNode(), Imm);
   }
   
  +/// isIntU8Immediate - This method tests to see if the node is either a 32-bit
  +/// or 64-bit immediate, and if the value can be accurately represented as a
  +/// zero (unsigned) extension from a 8-bit value.  If so, this returns true 
  +/// and the immediate.
  +bool llvm::isIntU8Immediate(SDNode *N, uint8_t &Imm) {
  +  if (!isa<ConstantSDNode>(N))
  +    return false;
  +  Imm = (uint8_t)cast<ConstantSDNode>(N)->getZExtValue();
  +  if (N->getValueType(0) == MVT::i32)
  +    return Imm == (int32_t)cast<ConstantSDNode>(N)->getZExtValue();
  +  else
  +    return Imm == (int64_t)cast<ConstantSDNode>(N)->getZExtValue();
  +}
  +bool llvm::isIntU8Immediate(SDValue Op, uint8_t &Imm) {
  +  return isIntU8Immediate(Op.getNode(), Imm);
  +}
   
  -/// SelectAddressEVXRegReg - Given the specified address, check to see if it can
  -/// be represented as an indexed [r+r] operation.
  +/// SelectAddressEVXRegReg - Given the specified address, check to see if it must
  +/// be represented as an indexed [r+r] operation for EVLDD and EVSTD instruction.
  +/// If the address offset is know now, it will be checked if it fits into the 
  +/// 8-bit offset, with an alignment of 8.
   bool PPCTargetLowering::SelectAddressEVXRegReg(SDValue N, SDValue &Base,
                                                  SDValue &Index,
                                                  SelectionDAG &DAG) const {
  @@ -2237,9 +2255,13 @@ bool PPCTargetLowering::SelectAddressEVX
         UI != E; ++UI) {
       if (MemSDNode *Memop = dyn_cast<MemSDNode>(*UI)) {
         if (Memop->getMemoryVT() == MVT::f64) {
  -          Base = N.getOperand(0);
  -          Index = N.getOperand(1);
  -          return true;
  +           uint8_t imm = 0;
  +        if (isIntU8Immediate(N.getOperand(1), imm)
  +            && !(imm % EVXEncodingAlignment))
  +          return false; // offset is okay for the 8-bit offset
  +        Base = N.getOperand(0);
  +        Index = N.getOperand(1);
  +        return true; // offset is unknown or too large. Use reg-reg
         }
       }
     }
  --- PPCISelLowering.orig.h      2019-07-01 09:08:11.462574400 +0200
  +++ PPCISelLowering.h   2019-07-01 08:39:14.625343900 +0200
  @@ -1199,6 +1200,11 @@ namespace llvm {
     bool isIntS16Immediate(SDNode *N, int16_t &Imm);
     bool isIntS16Immediate(SDValue Op, int16_t &Imm);
   
  +  bool isIntU8Immediate(SDNode *N, uint8_t &Imm);
  +  bool isIntU8Immediate(SDValue Op, uint8_t &Imm);
  +
  +  // The EVLDD and EVSTD are using 8bit offset with an aligment of 8
  +  const unsigned EVXEncodingAlignment = 8;
   } // end namespace llvm
   
   #endif // LLVM_TARGET_POWERPC_PPC32ISELLOWERING_H

I'm not sure about the

  const unsigned EVXEncodingAlignment = 8;

in the headerfile. I want to use a "name" for the alignment, instead of a hardcoded "8". What/where is the correct usage for such constants?

I don't have additional tests for it, as I haven't run the compiler tests and don't know how specify the tests. My local tests are to compile all Libraries I have for OS-9, several benchmarks (e.g. whetstone, dhrystone) and other tests to see if it is correct. I know that this test information is not enough here, but I can't handle it in a different way at the moment.

There are no open relocation issues seen from my side.

The only thing I haven't seen is the patch for the compares, I have made in PPCISelDAGToDAG.cpp in D54583 <https://reviews.llvm.org/D54583> on March 27. Without this patch, the FPU (SPE) compares are not correct. Or has this been addressed in a different way somewhere else (which I might have not seen)?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54409/new/

https://reviews.llvm.org/D54409