[PATCH] D8313: Refactor commoning of target specific load/store intrinsics in EarlyCSE
Sanjin Sijaric via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 6 12:33:08 PDT 2015
ssijaric added inline comments.
================
Comment at: include/llvm/Analysis/LoadStoreSite.h:47
@@ +46,3 @@
+ return;
+ if (NativeInstr *Inst = dyn_cast<NativeInstr>(V)) {
+ I.setPointer(Inst);
----------------
reames wrote:
> auto
Will change to auto throughout in the updated patch.
================
Comment at: include/llvm/Analysis/LoadStoreSite.h:99
@@ +98,3 @@
+ /// type of instruction. Given a load instruction, a compatible store
+ /// instruction is the one that is a mirror of the load instruction.
+ bool isCompatible(const Value *Other) const {
----------------
reames wrote:
> hfinkel wrote:
> > reames wrote:
> > > This API feels really awkward. For now, I'll ask that you pull this function out of the helper class and make it a static function in EarlyCSE. That at least localizes the awkwardness to the place it's used.
> > Philip, why do you feel this is awkward? It seems that if the point of the abstraction is to hide the differentiation between regular load/stores and related target intrinsics. As such, it seems that this is an important part of the abstraction.
> Er, I've forgotten it's been so long since I looked at this. This was a minor comment and can be safely ignored. This can be addressed post commit if needed; I really don't want to block submission of this given how long it's been outstanding.
I'll keep isCompatible in LoadStoreSite for now. I'll move matchInstructionAndLocation to EarlyCSE.
================
Comment at: include/llvm/Analysis/LoadStoreSite.h:122
@@ +121,3 @@
+
+ /// MatchInstructionAndLocation - true if Other is a compatible load or store
+ /// instruction, reading
----------------
reames wrote:
> Same as previous.
Will move it to EarlyCSE.
================
Comment at: include/llvm/Analysis/LoadStoreSite.h:125
@@ +124,3 @@
+ /// from, or writing to the same memory location.
+ bool MatchInstructionAndLocation(const LoadStoreSiteBase &Other) const {
+ assert(I.getPointer() && "Instruction not set");
----------------
hfinkel wrote:
> MatchInstructionAndLocation -> matchInstructionAndLocation
Will rename and move to EarlyCSE.
================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:501
@@ +500,3 @@
+ /// purposes.
+ bool areInlineCompatible(const Function *Caller,
+ const Function *Callee) const;
----------------
reames wrote:
> It looks like you may have merged two patches by accident here?
Git format-patch added a bit of additional context - removing areInlineCompatible, and then adding it back in. The function areInlineCompatible is part of TTI already.
================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:504
@@ +503,3 @@
+
+ /// \returns True if the target intrinsic behaves like a load instruction.
+ /// Must have one memory operand.
----------------
reames wrote:
> Important detail that needs to be in this comment: not all load like target instructions will return true. This is correct because the optimizer must treat intrinsics calls conservatively. i.e. this is an optimization hook the target can hook into, not a correctness hook.
Will update in the new patch.
================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:547
@@ +546,3 @@
+ ///
+ /// This allows the target to override the default mayWriteToMemory
+ /// query for those intrinsics not marked with readnone or readonly
----------------
reames wrote:
> Why not just mark the intrinsics with readonly or readnone? Unless you have a very good reason, please do that.
I did this mainly for consistency. I need mayTargetIntrinsicReadFromMemory to return false for store-like intrinsics (which can't have the readnone attribute set). So, I thought I'd introduce mayTargetIntrinsicWriteToMemory.
I'll remove mayTargetIntrinsicWriteToMemory in the updated patch, as load intrinsics coming from clang are already marked with the "readonly" attribute. (At least AArch64 ones are).
================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:557
@@ +556,3 @@
+ /// result by invoking mayTargetIntrinsicReadFromMemory.
+ bool mayReadFromMemory(const Instruction *I) const;
+
----------------
reames wrote:
> This is the wrong place for this API. It should probably be on LoadSiteBase. It doesn't need to be part of TTI.
I put this function (and mayWriteToMemory) here so that we can use a single check in EarlyCSE, as in:
if (TTI.mayReadFromMemory(Inst))
...
Otherwise, if it's moved to LoadStoreBase, we have to check along the lines:
StoreSite S(Inst, TTI);
if (Inst->mayReadFromMemory() && !(StoreSite && !StoreSite.mayReadFromMemory())
I can move mayReadFromMemory(cons Instruction *I) to EarlyCSE?
================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:564
@@ -530,1 +563,3 @@
+ /// result by invoking mayTargetIntrinsicWriteToMemory.
+ bool mayWriteToMemory(const Instruction *I) const;
----------------
reames wrote:
> Same as above.
Will move it to EarlyCSE.
================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:334
@@ +333,3 @@
+ bool isTargetIntrinsicAtomic(const IntrinsicInst *II) {
+ assert(false &&
+ "Target does not support querying of target memory intrinsics.");
----------------
reames wrote:
> A slightly clearer way to phrase these assertions would be that the intrinsic passes either the LikeLoad or LikeStore predicate. This allows a target to override those two and not have to update the others to get a conservatively correct answer.
I will remove the assert for isTargetIntrinsicAtomic, and call II->isAtomic() for the default implementation. For isTargetIntrinsicVolatile, I'll add the asserts if an intrinsic isn't like a load or a store, and return true otherwise to err on the conservative side.
I will keep asserts for getTargetIntrinsicPointerOperand, getOrCreateResultFromTargetIntrinsic and getTargetIntrinsicMatchingId, as these must be implemented by the target if they are called.
================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:485
@@ +484,3 @@
+// AArch64 load/store intrinsics are not atomic.
+bool AArch64TTIImpl::isTargetIntrinsicAtomic(const IntrinsicInst *Inst) {
+ return false;
----------------
reames wrote:
> You should assert that the intrinsic is in either the load or store list. Otherwise, this could return an incorrect result for an unknown intrinsic.
>
> Alternatively, it could be:
> if intrinsic in [known list] return false;
> else return true;
I will add a check for load/store intrinsic for isTargetIntrinsicVolatile. For isTargetIntrinsicAtomic, I'll return Inst->isAtomic() in the base TTI implementation.
================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:490
@@ +489,3 @@
+// AArch64 load/store intrinsics are not volatile.
+bool AArch64TTIImpl::isTargetIntrinsicVolatile(const IntrinsicInst *Inst) {
+ return false;
----------------
reames wrote:
> Same.
Will change this in the updated patch.
================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:504
@@ -470,3 +503,3 @@
// Create a struct type
- StructType *ST = dyn_cast<StructType>(ExpectedType);
+ const StructType *ST = dyn_cast<StructType>(ExpectedType);
if (!ST)
----------------
reames wrote:
> Submit separately please.
Will change in the updated patch.
================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:566
@@ +565,3 @@
+ // TODO: Check for atomicity and volatility.
+ if (isTargetIntrinsicLikeStore(Inst))
+ return false;
----------------
reames wrote:
> If you rephrase the assertions above, you can just call the existing check functions and remove all these comments. Please do so.
I'll add a check for volatility and atomicity in mayTargetIntrinsicReadFromMemory in the updated patch, and remove the comments. I'll remove mayTargetIntrinsicWriteToMemory altogether as load intrinsics are marked as readonly.
================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:295
@@ -293,3 +294,3 @@
BumpPtrAllocator,
- ScopedHashTableVal<Value *, std::pair<Value *, unsigned>>>
+ ScopedHashTableVal<const Value *, std::pair<Value *, unsigned>>>
LoadMapAllocator;
----------------
reames wrote:
> Separable change. Please submit separately.
Will do in the updated patch.
================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:528
@@ -585,8 +527,3 @@
// If this instruction may read from memory, forget LastStore.
- // Load/store intrinsics will indicate both a read and a write to
- // memory. The target may override this (e.g. so that a store intrinsic
- // does not read from memory, and thus will be treated the same as a
- // regular store for commoning purposes).
- if (Inst->mayReadFromMemory() &&
- !(MemInst.isValid() && !MemInst.mayReadFromMemory()))
+ if (TTI.mayReadFromMemory(Inst))
LastStore = nullptr;
----------------
reames wrote:
> Ah, given where this is used, ignore my comment about putting this on LoadStoreBase. Making it a static function in EarlyCSE is better for the moment. Still not on TTI, but LSB isn't a good place either.
Will make it a static function in the updated patch.
================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:560
@@ -623,1 +559,3 @@
+ StoreSite SS(Inst, TTI);
+ if (SS) {
// We do a trivial form of DSE if there are two stores to the same
----------------
reames wrote:
> Use the assignment idiom as above.
Will fix in the updated patch.
http://reviews.llvm.org/D8313
More information about the llvm-commits
mailing list