[PATCH] D31539: Hoisting invariant.group in LICM

[Sanjoy Das via Phabricator via llvm-commits]

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