[PATCH] Refactor commoning of target specific load/store intrinsics in EarlyCSE
Sanjin Sijaric
ssijaric at codeaurora.org
Thu Apr 2 01:22:08 PDT 2015
Hi Philip,
Thanks for reviewing these changes! I'll upload a new patch with context.
================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:471
@@ +470,3 @@
+ /// access.
+ bool isTargetMemoryIntrinsicAtomic(const IntrinsicInst *II) const;
+
----------------
reames wrote:
> The naming of these functions is inconsistent. I'm fine with either scheme, but please be consistent.
The inconsistency in naming is to indicate that isTargetIntrinsicLikeLoad may be invoked on any intrinsic instruction, whereas isTargetMemoryIntrinsic..... may be invoked on intrinsics that are either like loads or like stores. The asserts in TargetTransformInfo.cpp check for these properties.
I renamed these from isTargetMemoryIntrinsic* to isTargetIntrinsic* in the new patch. I kept the asserts for now. Maybe they should be renamed to something like isTargetLoadOrStoreIntrinsicAtomic instead?
Alternatively, we could do away with different query functions, and just have something like
MemoryIntrinsicInfo getTargetMemoryIntrinsic(IntrinsicInst*);
with overloaded operator bool(), which would return true if the MemoryIntrinsicInfo contains a supported intrinsic. The overhead would be that we would probably need to keep one MemoryIntrinsicInfo struct in LoadStoreSiteBase. This could perhaps avoid any inconsistencies due to having different query functions.
================
Comment at: include/llvm/IR/LoadStoreSite.h:1
@@ +1,2 @@
+//===- LoadStoreSite.h - Abstract Load and Store instructions ---*- C++ -*-===//
+//
----------------
reames wrote:
> I might call this something like MemoryAccessSite.h or the like. Mild preference, don't bother changing this until other things are settled.
Yes, let's deal with renaming once other issues are addressed.
================
Comment at: include/llvm/IR/LoadStoreSite.h:33
@@ +32,3 @@
+ typename IntrinsicInstTy = IntrinsicInst>
+class LoadStoreSiteBase {
+protected:
----------------
reames wrote:
> I might call this MemoryAccessBase, but that's a minor preference. My thought process is that it allows a logic extension for an AtomicRMWSite at some point in the future.
As per your previous suggestion, let's deal with renaming once other issues are addressed.
================
Comment at: include/llvm/IR/LoadStoreSite.h:69
@@ +68,3 @@
+ InstrTy *Inst = I.getPointer();
+ if (I.getInt())
+ return cast<NativeInstr>(Inst)->getPointerOperand();
----------------
reames wrote:
> I'd wrap getInt in a helper function which makes the purpose clearer to the reader. The following seems cleaner and easier to read:
>
> if (I.isTargetIntrinsic())
> TTI->getTargetMemoryIntrinsicPointerOperand(
> cast<IntrinsicInstTy>(Inst));
> return cast<NativeInstr>(Inst)->getPointerOperand();
Done in the updated patch.
================
Comment at: include/llvm/IR/LoadStoreSite.h:76
@@ +75,3 @@
+ LoadStoreSiteBase &operator=(InstrTy *Inst) {
+ I.setPointer(nullptr);
+ I.setInt(false);
----------------
reames wrote:
> You've got this logic duplicated in a few places. Please common.
Commoned in the updated patch.
================
Comment at: include/llvm/IR/LoadStoreSite.h:99
@@ +98,3 @@
+ return Inst->mayReadFromMemory();
+ return TTI->mayTargetMemoryIntrinsicReadFromMemory(
+ cast<IntrinsicInst>(Inst));
----------------
reames wrote:
> Having this here is sorta okay, but it really feels like this needs to live somewhere that applies easily to all intrinsics. We have the readonly/readnone attributes, why not just mark the underling intrinsics appropriately?
On AArch64 and ARM at least, store intrinsics are not marked as readnone by the front-end, and so mayReadFromMemory would always return true. For regular store instructions, mayReadFromMemory always returns false for non-atomic stores. This change allows the back-end to treat store intrinsics like regular stores.
I don't think it's legal to mark store intrinsics with readnone. When readnone is used on a function, then the function "does not write through any pointer arguments", according to the LLVM Language Reference Manual.
================
Comment at: include/llvm/IR/LoadStoreSite.h:110
@@ +109,3 @@
+ if (const LoadInst *LI = dyn_cast<LoadInst>(Other))
+ return getPointerOperand()->getType() ==
+ LI->getPointerOperand()->getType();
----------------
reames wrote:
> This shouldn't be necessary. If EarlyCSE doesn't know how to insert bitcasts, it really, really should.
I put this here for completeness, in case some other optimization makes use of this API. For example, it would return false when checking "store i32 %val, i32* %ptr" and "load %val2 = load i16, i16* %ptr2".
This mirrors the check for
%0 = bitcast i32* %a to i8*
call void @llvm.aarch64.neon.st2.v4i32.p0i8(<4 x i32> %3, <4 x i32> %4, i8* %0)
and
%5 = bitcast i32* %a to i8*
%vld3 = call { <4 x i32>, <4 x i32>, <4 x i32> } @llvm.aarch64.neon.ld3.v4i32.p0i8(i8* %5)
While the check for non-intrinsic loads and stores is not necessary when using this API in EaryCSE, I don't think I should just return "true" unconditionally for regular loads and stores.
================
Comment at: include/llvm/IR/LoadStoreSite.h:146
@@ +145,3 @@
+ }
+ static bool isTargetMemoryIntrinsicSupported(const IntrinsicInst *II,
+ const TargetTransformInfo *TTI) {
----------------
reames wrote:
> This should probably be private.
Changed in the updated patch.
================
Comment at: lib/Analysis/TargetTransformInfo.cpp:269
@@ +268,3 @@
+
+bool TargetTransformInfo::isTargetMemoryIntrinsicAtomic(
+ const IntrinsicInst *II) const {
----------------
reames wrote:
> What do you mean by "Atomic" here? AtomicRMW (the instruction?). Atomic (as in memory ordering?) The naming is very unclear.
This is supposed to check for memory ordering, similar to isAtomic() when invoked on regular loads and stores. The new name, isTargetIntrinsicAtomic(), is probably not any better. Is the comment in TargetTransformInfo.h enough?
================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:539
@@ +538,3 @@
+ const IntrinsicInst *Inst) {
+ if (isTargetIntrinsicLikeLoad(Inst))
+ return true;
----------------
reames wrote:
> Surely there are other intrinsics which read from memory? I really doubt the correctness of this code.
There are, but only a few intrinsics are supported for now (the ones in isTargetIntrinsicLikeLoad(...) and isTargetIntrinsicLikeStore(...)). The assert makes sure that we are dealing with supported load/store intrinsics.
Perhaps, a better way would be to just have one query function for load/store intrinsics. Something like
MemoryIntrinsicInfo getTargetMemoryIntrinsic(IntrinsicInst *Inst);
which would return all the needed information inside the MemoryIntrinsicInfo struct? Then we could do away with separate APIs, which may be confusing. The overhead would be the inclusion of this struct in LoadStoreSiteBase.
================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:469
@@ -526,3 +468,3 @@
// Ignore volatile loads.
- if (MemInst.isVolatile()) {
+ if (!LS.isSimple()) {
LastStore = nullptr;
----------------
reames wrote:
> Volatile != Simple. Please use appropriate naming to match underling instruction.
Originally, the check was
// Ignore volatile loads.
if (!LI->isSimple())
The check "if (MemInst.isVolatile())" does the same check underneath, at least for regular loads and stores. The name of MemInst.isVolatile() is wrong - it should have been MemInst.isSimple()).
The new check
if (!LS.isSimple()) restores the original check that was in place.
The comment , "// Ignore volatile loads." should probably be "// Ignore volatile and atomic loads".
================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:482
@@ -540,1 +481,3 @@
+ if (InVal.first != nullptr && InVal.second == CurrentGeneration &&
+ LS.isCompatible(InVal.first)) {
Value *Op = getOrCreateResult(InVal.first, Inst->getType());
----------------
reames wrote:
> Er, is this a separate bugfix? I don't understand where this is coming from w.r.t. the requested refactor.
Yes, this bug was pointed out by Hal, and could affect Power intrinsics from what I understand. I thought I'd include it here. If you prefer, I can do this under a seperate bug fix once the refactor is sorted out.
================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:510
@@ -567,1 +509,3 @@
+ StoreSite SS(Inst, TTI);
+ if (Inst->mayReadFromMemory() && !(SS && !SS.mayReadFromMemory()))
LastStore = nullptr;
----------------
reames wrote:
> It really feels like the functionality for whether an intrinsic reads from memory should live inside mayReadFromMemory.
AArch64 store intrinsics are not marked as readnone, and mayReadFromMemory will always return true (unlike for regular non-atomic stores). So, we wouldn't get a chance to common intrinsic stores. It doesn't look to be legal to mark store intrinsics as readnone, according to the LLVM Language Reference Manual.
================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:545
@@ -600,5 +544,3 @@
if (LastStore) {
- ParseMemoryInst LastStoreMemInst(LastStore, TTI);
- if (LastStoreMemInst.isMatchingMemLoc(MemInst)) {
- DEBUG(dbgs() << "EarlyCSE DEAD STORE: " << *LastStore
- << " due to: " << *Inst << '\n');
+ if (LastStore.Match(SS)) {
+ DEBUG(dbgs() << "EarlyCSE DEAD STORE: "
----------------
reames wrote:
> Given this is testing whether a location matches not whether the instruction matches, I would like to see something which made that explicit.
The previous change, isMatchingMemLoc, has a misleading name. It checks for both the memory operand and the matching id. This is what LoadStoreSiteBase::Match does as well. I renamed LoadStoreSiteBase::Match to LoadStoreSiteBase::MatchInstructionAndLocation in the new patch.
================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:567
@@ -621,3 +566,3 @@
// Remember that this was the last store we saw for DSE.
- if (!MemInst.isVolatile())
+ if (SS.isSimple())
LastStore = Inst;
----------------
reames wrote:
> a) naming, b) inverted conditional (bug!)
MemInst.isVolatile() is not correctly named - it checks for !isSimple() for regular loads and stores.
Previous to MemInst change, the check was
if (SI->isSimple())
The new change, if (SS.isSimple()), restores the original check.
http://reviews.llvm.org/D8313
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list