Eliminate TargetTransformInfo::isFoldableMemAccess()

[jonpa]

Summary

isLegalAddressingMode() has recently gained the extra optional Instruction* parameter, and therefore it can now do the job that previously isFoldableMemAccess() could only do.

The SystemZ implementation of isLegalAddressingMode() has gained the functionality of checking for offsets, which used to be done with isFoldableMemAccess().

The isFoldableMemAccess() hook can be removed everywhere.

I used the isAMCompletelyFolded() wrapper again in LoopStrengthReduce.cpp, to avoid duplicating code just like last time.

[uweigand]

The SystemZ part looks correct to me, see the inline comment for a style/readability suggestion.

I'm not sure about the common code changes -- what does simply calling isLegalAddressingMode instead of isFoldableMemAccessOffset do to targets that have not yet added support to do instruction-specific checks to the former? I'd assume you'd see performance regressions there.

Maybe this transition should be done on a target-by-target basis.

lib/Target/SystemZ/SystemZISelLowering.cpp

710–711

Hmm, this pulls the conditions apart again ... Maybe readability would be improved by just always having a SupportedAM variable, initialized at the top like:

AddressingMode SupportedAM(true, true);
if (I != nullptr)
   SupportedAM = supportedAddressingMode(I, Subtarget.hasVector());

and then just use it throughout the function.

[jonpa]

I'm not sure about the common code changes -- what does simply calling isLegalAddressingMode instead of isFoldableMemAccessOffset do to targets that have not yet added support to do instruction-specific checks to the former? I'd assume you'd see performance regressions there.

Maybe this transition should be done on a target-by-target basis.

SystemZ was the only user of isFoldableMemAccessOffset() except for any out-of-tree targets. I was thinking that this transition should not be much more difficult than what it was on SystemZ. Since it is now clear that this method is obsolete, perhaps a heads-up in the commit message will do?

Hmm, this pulls the conditions apart again ... Maybe readability would be improved by just always having a SupportedAM variable, initialized at the top like...

Done.

[uweigand]

In D35933#823813, @jonpa wrote:

I'm not sure about the common code changes -- what does simply calling isLegalAddressingMode instead of isFoldableMemAccessOffset do to targets that have not yet added support to do instruction-specific checks to the former? I'd assume you'd see performance regressions there.

Maybe this transition should be done on a target-by-target basis.

SystemZ was the only user of isFoldableMemAccessOffset() except for any out-of-tree targets.

Well, even if they had no separate implementation, they would fall back to the default one (returning always true). Now the code will do a call to isLegalAddressingMode instead. Is is clear that this will either be no change (i.e. also return always true), or else an lead to an actual improvement, on all other targets?

I'm just wondering whether this could expose a performance regression on some other target ...

[jonpa]

In D35933#823866, @uweigand wrote:

In D35933#823813, @jonpa wrote:

I'm not sure about the common code changes -- what does simply calling isLegalAddressingMode instead of isFoldableMemAccessOffset do to targets that have not yet added support to do instruction-specific checks to the former? I'd assume you'd see performance regressions there.

Maybe this transition should be done on a target-by-target basis.

SystemZ was the only user of isFoldableMemAccessOffset() except for any out-of-tree targets.

Well, even if they had no separate implementation, they would fall back to the default one (returning always true). Now the code will do a call to isLegalAddressingMode instead. Is is clear that this will either be no change (i.e. also return always true), or else an lead to an actual improvement, on all other targets?

I'm just wondering whether this could expose a performance regression on some other target ...

I see your point now. Of course, if this does change things around it should hopefully be for the better.

Given all the other calls to isAMCompletelyFolded() and so on, I have never been quite sure that other targets ever needed this extra checking based on checking the actual Instructions like SystemZ. That's why I recently proposed guarding this with TTI.LSRWithInstrQueries(), which is still an alternative, I guess. I think Quentin was against this for the sake of out-of-tree targets, IIRC.

Quentin, what is your opinion now?

[qcolombet]

Hi Jonas,

Looks reasonable to me.

Thanks for killing some hooks :).

Cheers,
-Quentin

[jonpa]

Just before I was about to commit, I noticed that a Hexagon test broke (test/CodeGen/Hexagon/swp-const-tc.ll), so unfortunately this will have to wait a bit, since I am now on vacation.

[jonpa]

The Hexagon test passes by just updating the label like #998->#999. It seems that the loop is now one instruction smaller also, so I am assuming everyone is happy with this.

Diff on output (old <> new)

10,11c10
< loop0(.LBB0_1,#998)

< r2 = #-4000

			loop0(.LBB0_1,#999)

15,21c14,15
< r3 = add(r0,r2)
< r4 = add(r2,#4)
< }
< {
< r2 = add(r0,r4)
< r4 = add(r4,#4)

< r3 = memw(r3+#4000)

			r3 = add(r1,#4)
			r2 = memw(r0+r1<<#0)

27,30c21,23
< r1 = add(r3,r1)
< r2 = add(r0,r4)
< r4 = add(r4,#4)

< r3 = memw(r2+#4000)

			r1 = add(r2,r1)
			r3 = add(r3,#4)
			r2 = memw(r0+r3<<#0)

34,38c27
< r0 = add(r3,r1)
< r2 = memw(r2+#4000)
< }
< {

< r0 = add(r2,r0)

			r0 = add(r2,r1)

[jonpa]

Properly formatted diff:

10,11c10
< 			loop0(.LBB0_1,#998)
< 			r2 = #-4000
---
> 			loop0(.LBB0_1,#999)
15,21c14,15
< 			r3 = add(r0,r2)
< 			r4 = add(r2,#4)
< 	}
< 	{
< 			r2 = add(r0,r4)
< 			r4 = add(r4,#4)
< 			r3 = memw(r3+#4000)
---
> 			r3 = add(r1,#4)
> 			r2 = memw(r0+r1<<#0)
27,30c21,23
< 			r1 = add(r3,r1)
< 			r2 = add(r0,r4)
< 			r4 = add(r4,#4)
< 			r3 = memw(r2+#4000)
---
> 			r1 = add(r2,r1)
> 			r3 = add(r3,#4)
> 			r2 = memw(r0+r3<<#0)
34,38c27
< 			r0 = add(r3,r1)
< 			r2 = memw(r2+#4000)
< 	}
< 	{
< 			r0 = add(r2,r0)
---
> 			r0 = add(r2,r1)

[jonpa]

Thanks for review.
trunk@310463

include/llvm/Analysis/TargetTransformInfo.h

int getScalingFactorCost(Type *Ty, GlobalValue *BaseGV, int64_t BaseOffset,
                            unsigned AddrSpace = 0) const;
 
   /// \brief Return true if the loop strength reduce pass should make
   /// Instruction* based TTI queries to isLegalAddressingMode(). This is
   /// needed on SystemZ, where e.g. a memcpy can only have a 12 bit unsigned
   /// immediate offset and no index register.
   bool LSRWithInstrQueries() const;
 
-  /// \brief Return true if target supports the load / store
-  /// instruction with the given Offset on the form reg + Offset. It
-  /// may be that Offset is too big for a certain type (register
-  /// class).
-  bool isFoldableMemAccessOffset(Instruction *I, int64_t Offset) const;
-  
   /// \brief Return true if it's free to truncate a value of type Ty1 to type
   /// Ty2. e.g. On x86 it's free to truncate a i32 value in register EAX to i16
   /// by referencing its sub-register AX.
   bool isTruncateFree(Type *Ty1, Type *Ty2) const;
 
   /// \brief Return true if it is profitable to hoist instruction in the
   /// then/else to before if.
   bool isProfitableToHoist(Instruction *I) const;
public:
   virtual bool isLegalMaskedLoad(Type *DataType) = 0;
   virtual bool isLegalMaskedScatter(Type *DataType) = 0;
   virtual bool isLegalMaskedGather(Type *DataType) = 0;
   virtual bool prefersVectorizedAddressing() = 0;
   virtual int getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
                                    int64_t BaseOffset, bool HasBaseReg,
                                    int64_t Scale, unsigned AddrSpace) = 0;
   virtual bool LSRWithInstrQueries() = 0;
-  virtual bool isFoldableMemAccessOffset(Instruction *I, int64_t Offset) = 0;
   virtual bool isTruncateFree(Type *Ty1, Type *Ty2) = 0;
   virtual bool isProfitableToHoist(Instruction *I) = 0;
   virtual bool isTypeLegal(Type *Ty) = 0;
   virtual unsigned getJumpBufAlignment() = 0;
   virtual unsigned getJumpBufSize() = 0;
   virtual bool shouldBuildLookupTables() = 0;
   virtual bool shouldBuildLookupTablesForConstant(Constant *C) = 0;
   virtual unsigned
int getScalingFactorCost(Type *Ty, GlobalValue *BaseGV, int64_t BaseOffset,
                            bool HasBaseReg, int64_t Scale,
                            unsigned AddrSpace) override {
     return Impl.getScalingFactorCost(Ty, BaseGV, BaseOffset, HasBaseReg,
                                      Scale, AddrSpace);
   }
   bool LSRWithInstrQueries() override {
     return Impl.LSRWithInstrQueries();
   }
-  bool isFoldableMemAccessOffset(Instruction *I, int64_t Offset) override {
-    return Impl.isFoldableMemAccessOffset(I, Offset);
-  }
   bool isTruncateFree(Type *Ty1, Type *Ty2) override {
     return Impl.isTruncateFree(Ty1, Ty2);
   }
   bool isProfitableToHoist(Instruction *I) override {
     return Impl.isProfitableToHoist(I);
   }
   bool isTypeLegal(Type *Ty) override { return Impl.isTypeLegal(Ty); }
   unsigned getJumpBufAlignment() override { return Impl.getJumpBufAlignment(); }

include/llvm/Analysis/TargetTransformInfoImpl.h

int getScalingFactorCost(Type *Ty, GlobalValue *BaseGV, int64_t BaseOffset,
     if (isLegalAddressingMode(Ty, BaseGV, BaseOffset, HasBaseReg,
                               Scale, AddrSpace))
       return 0;
     return -1;
   }
 
   bool LSRWithInstrQueries() { return false; }
 
-  bool isFoldableMemAccessOffset(Instruction *I, int64_t Offset) { return true; }
-
   bool isTruncateFree(Type *Ty1, Type *Ty2) { return false; }
 
   bool isProfitableToHoist(Instruction *I) { return true; }
 
   bool isTypeLegal(Type *Ty) { return false; }
 
   unsigned getJumpBufAlignment() { return 0; }
 

include/llvm/CodeGen/BasicTTIImpl.h

int getScalingFactorCost(Type *Ty, GlobalValue *BaseGV, int64_t BaseOffset,
     TargetLoweringBase::AddrMode AM;
     AM.BaseGV = BaseGV;
     AM.BaseOffs = BaseOffset;
     AM.HasBaseReg = HasBaseReg;
     AM.Scale = Scale;
     return getTLI()->getScalingFactorCost(DL, AM, Ty, AddrSpace);
   }
 
-  bool isFoldableMemAccessOffset(Instruction *I, int64_t Offset) {
-    return getTLI()->isFoldableMemAccessOffset(I, Offset);
-  }
-
   bool isTruncateFree(Type *Ty1, Type *Ty2) {
     return getTLI()->isTruncateFree(Ty1, Ty2);
   }
 
   bool isProfitableToHoist(Instruction *I) {
     return getTLI()->isProfitableToHoist(I);
   }
 

include/llvm/Target/TargetLowering.h

public:
   virtual int getScalingFactorCost(const DataLayout &DL, const AddrMode &AM,
                                    Type *Ty, unsigned AS = 0) const {
     // Default: assume that any scaling factor used in a legal AM is free.
     if (isLegalAddressingMode(DL, AM, Ty, AS))
       return 0;
     return -1;
   }
 
-  virtual bool isFoldableMemAccessOffset(Instruction *I, int64_t Offset) const {
-    return true;
-  }
-
   /// Return true if the specified immediate is legal icmp immediate, that is
   /// the target has icmp instructions which can compare a register against the
   /// immediate without having to materialize the immediate into a register.
   virtual bool isLegalICmpImmediate(int64_t) const {
     return true;
   }
 
   /// Return true if the specified immediate is legal add immediate, that is the

lib/Analysis/TargetTransformInfo.cpp

int TargetTransformInfo::getScalingFactorCost(Type *Ty, GlobalValue *BaseGV,
   assert(Cost >= 0 && "TTI should not produce negative costs!");
   return Cost;
 }
 
 bool TargetTransformInfo::LSRWithInstrQueries() const {
   return TTIImpl->LSRWithInstrQueries();
 }
 
-bool TargetTransformInfo::isFoldableMemAccessOffset(Instruction *I,
-                                                    int64_t Offset) const {
-  return TTIImpl->isFoldableMemAccessOffset(I, Offset);
-}
-
 bool TargetTransformInfo::isTruncateFree(Type *Ty1, Type *Ty2) const {
   return TTIImpl->isTruncateFree(Ty1, Ty2);
 }
 
 bool TargetTransformInfo::isProfitableToHoist(Instruction *I) const {
   return TTIImpl->isProfitableToHoist(I);
 }
 

lib/Target/SystemZ/SystemZISelLowering.h

EVT getSetCCResultType(const DataLayout &DL, LLVMContext &,
                          EVT) const override;
   bool isFMAFasterThanFMulAndFAdd(EVT VT) const override;
   bool isFPImmLegal(const APFloat &Imm, EVT VT) const override;
   bool isLegalICmpImmediate(int64_t Imm) const override;
   bool isLegalAddImmediate(int64_t Imm) const override;
   bool isLegalAddressingMode(const DataLayout &DL, const AddrMode &AM, Type *Ty,
                              unsigned AS,
                              Instruction *I = nullptr) const override;
-  bool isFoldableMemAccessOffset(Instruction *I, int64_t Offset) const override;
   bool allowsMisalignedMemoryAccesses(EVT VT, unsigned AS,
                                       unsigned Align,
                                       bool *Fast) const override;
   bool isTruncateFree(Type *, Type *) const override;
   bool isTruncateFree(EVT, EVT) const override;
   const char *getTargetNodeName(unsigned Opcode) const override;
   std::pair<unsigned, const TargetRegisterClass *>
   getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,

lib/Target/SystemZ/SystemZISelLowering.cpp

if (HasVector && (isa<LoadInst>(I) || isa<StoreInst>(I))) {
 
     if (IsFPAccess || IsVectorAccess)
       return AddressingMode(false/*LongDispl*/, true/*IdxReg*/);
   }
 
   return AddressingMode(true/*LongDispl*/, true/*IdxReg*/);
 }
 
-// TODO: This method should also check for the displacement when *I is
-// passed. It may also be possible to merge with isFoldableMemAccessOffset()
-// now that both methods get the *I.
 bool SystemZTargetLowering::isLegalAddressingMode(const DataLayout &DL,
-            const AddrMode &AM, Type *Ty, unsigned AS, Instruction *I) const {
   // Punt on globals for now, although they can be used in limited
   // RELATIVE LONG cases.
   if (AM.BaseGV)
     return false;
 
   // Require a 20-bit signed offset.
   if (!isInt<20>(AM.BaseOffs))
     return false;
 
-  if (I != nullptr &&
-      !supportedAddressingMode(I, Subtarget.hasVector()).IndexReg)
+  AddressingMode SupportedAM(true, true);
+  if (I != nullptr)
+    SupportedAM = supportedAddressingMode(I, Subtarget.hasVector());
+
+  if (!SupportedAM.LongDisplacement && !isUInt<12>(AM.BaseOffs))
+    return false;
+
+  if (!SupportedAM.IndexReg)
     // No indexing allowed.
     return AM.Scale == 0;
   else
     // Indexing is OK but no scale factor can be applied.
     return AM.Scale == 0 || AM.Scale == 1;
 }
 
-// TODO: Should we check for isInt<20> also?
-bool SystemZTargetLowering::isFoldableMemAccessOffset(Instruction *I,
-                                                      int64_t Offset) const {
-  if (!supportedAddressingMode(I, Subtarget.hasVector()).LongDisplacement)
-    return (isUInt<12>(Offset));
-
-  return true;
-}
-
 bool SystemZTargetLowering::isTruncateFree(Type *FromType, Type *ToType) const {
   if (!FromType->isIntegerTy() || !ToType->isIntegerTy())
     return false;
   unsigned FromBits = FromType->getPrimitiveSizeInBits();
   unsigned ToBits = ToType->getPrimitiveSizeInBits();
   return FromBits > ToBits;
 }
 
 bool SystemZTargetLowering::isTruncateFree(EVT FromVT, EVT ToVT) const {

lib/Transforms/Scalar/LoopStrengthReduce.cpp

public:
   void RecomputeRegs(size_t LUIdx, RegUseTracker &Reguses);
 
   void print(raw_ostream &OS) const;
   void dump() const;
 };
 
 } // end anonymous namespace
 
+static bool isAMCompletelyFolded(const TargetTransformInfo &TTI,
+                                 LSRUse::KindType Kind, MemAccessTy AccessTy,
+                                 GlobalValue *BaseGV, int64_t BaseOffset,
+                                 bool HasBaseReg, int64_t Scale,
+                                 Instruction *Fixup = nullptr);
+
 /// Tally up interesting quantities from the given register.
 void Cost::RateRegister(const SCEV *Reg,
                         SmallPtrSetImpl<const SCEV *> &Regs,
                         const Loop *L,
                         ScalarEvolution &SE, DominatorTree &DT) {
   if (const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(Reg)) {
     // If this is an addrec for another loop, it should be an invariant
     // with respect to L since L is the innermost loop (at least
if (F.BaseGV)
       C.ImmCost += 64; // Handle symbolic values conservatively.
                      // TODO: This should probably be the pointer size.
     else if (Offset != 0)
       C.ImmCost += APInt(64, Offset, true).getMinSignedBits();
 
     // Check with target if this offset with this instruction is
     // specifically not supported.
     if (LU.Kind == LSRUse::Address && Offset != 0 &&
-        !TTI.isFoldableMemAccessOffset(Fixup.UserInst, Offset))
+        !isAMCompletelyFolded(TTI, LSRUse::Address, LU.AccessTy, F.BaseGV,
+                              Offset, F.HasBaseReg, F.Scale, Fixup.UserInst))
       C.NumBaseAdds++;
   }
 
   // If we don't count instruction cost exit here.
   if (!InsnsCost) {
     assert(isValid() && "invalid cost");
     return;
   }
LLVM_DUMP_METHOD void LSRUse::dump() const {
   print(errs()); errs() << '\n';
 }
 #endif
 
 static bool isAMCompletelyFolded(const TargetTransformInfo &TTI,
                                  LSRUse::KindType Kind, MemAccessTy AccessTy,
                                  GlobalValue *BaseGV, int64_t BaseOffset,
                                  bool HasBaseReg, int64_t Scale,
-                                 Instruction *Fixup = nullptr) {
+                                 Instruction *Fixup/*= nullptr*/) {
   switch (Kind) {
   case LSRUse::Address:
     return TTI.isLegalAddressingMode(AccessTy.MemTy, BaseGV, BaseOffset,
                                      HasBaseReg, Scale, AccessTy.AddrSpace, Fixup);
 
   case LSRUse::ICmpZero:
     // There's not even a target hook for querying whether it would be legal to
     // fold a GV into an ICmp.