[PATCH] D31539: Hoisting invariant.group in LICM
Sanjoy Das via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 31 18:59:36 PDT 2017
sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.
Comments inline.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:495
+ // TODO can I do this without casting to Instruction?
+ if (auto *PointerOperandInst = dyn_cast<Instruction>(LI->getPointerOperand()))
+ return DT->properlyDominates(PointerOperandInst->getParent(),
----------------
You can use `Loop::isLoopInvariant` here.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:571
+ if (isLoadInvariantGroupInLoop(LI, DT, CurLoop))
+ return true;
----------------
I'm not sure that that the langref lets you do this. What it allows for is
```
%a = *ptr, !invariant.group
%b = *ptr, !invariant.group
```
to
```
%a = *ptr, !invariant.group
%b = %a
```
which is slightly different from what you're doing here.
So I'd recommend changing the langref wording to be more like what we have for `!invariant.load`.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:877
// drop. It is a compile time optimization, not required for correctness.
!isGuaranteedToExecute(I, DT, CurLoop, SafetyInfo))
+ I.dropUnknownNonDebugMetadata({LLVMContext::MD_invariant_group,
----------------
This bit does not look correct. Why can't these attributes be control dependent?
================
Comment at: test/Transforms/LICM/hoist-invariant-group-load.ll:39
+; CHECK-LABEL: define void @hoist2(
+define void @hoist2(i8** dereferenceable(8)) {
+entry:
----------------
Please use `-instnamer` to name all the instructions. Otherwise editing the tests will be painful.
https://reviews.llvm.org/D31539
More information about the llvm-commits
mailing list