[PATCH] [SROA] Fold a PHI node if all its incoming values are the same
Jingyue Wu
jingyue at google.com
Sat Jul 26 07:55:43 PDT 2014
================
Comment at: lib/Transforms/Scalar/SROA.cpp:332
@@ +331,3 @@
+static Value *foldPHINode(PHINode &PN) {
+ Value *CommonValue = UndefValue::get(PN.getType());
+ for (unsigned I = 0, E = PN.getNumIncomingValues(); I != E; ++I) {
----------------
Chandler Carruth wrote:
> Just use nullptr as the initial value?
That would make foldPHINode return null for phi(undef, undef).
================
Comment at: lib/Transforms/Scalar/SROA.cpp:333
@@ +332,3 @@
+ Value *CommonValue = UndefValue::get(PN.getType());
+ for (unsigned I = 0, E = PN.getNumIncomingValues(); I != E; ++I) {
+ Value *IV = PN.getIncomingValue(I);
----------------
Chandler Carruth wrote:
> I prefer to only use "I" (capitalized) for iterators. I'd use 'i' or 'Idx' here.
>
> (and we should really add an iterator and range adaptor set to PHINode for these so you can use a range based for loop...)
Done.
I agree. Will add a range for PHINode in a separate patch.
================
Comment at: lib/Transforms/Scalar/SROA.cpp:336-344
@@ +335,11 @@
+ // Treat UndefValue as a wildcard.
+ if (isa<UndefValue>(IV) || isa<UndefValue>(CommonValue)) {
+ if (!isa<UndefValue>(IV))
+ CommonValue = IV;
+ continue;
+ }
+ if (CommonValue != IV) {
+ CommonValue = nullptr;
+ break;
+ }
+ }
----------------
Chandler Carruth wrote:
> I think this would be slightly simpler as:
>
> if (isa<UndefValue>(IV))
> continue;
>
> if (!CommanValue)
> CommonValue = IV;
> else if (CommonValue != IV)
> return nullptr;
Done
================
Comment at: lib/Transforms/Scalar/SROA.cpp:676-681
@@ -662,11 +675,8 @@
- // For PHI and select operands outside the alloca, we can't nuke the entire
- // phi or select -- the other side might still be relevant, so we special
- // case them here and use a separate structure to track the operands
- // themselves which should be replaced with undef.
- // FIXME: This should instead be escaped in the event we're instrumenting
- // for address sanitization.
- if (Offset.uge(AllocSize)) {
- S.DeadOperands.push_back(U);
- return;
+ Value *Result = nullptr;
+ if (PHINode *PN = dyn_cast<PHINode>(&I)) {
+ Result = foldPHINode(*PN);
+ } else {
+ Result = foldSelectInst(cast<SelectInst>(I));
}
+ if (Result != nullptr) {
----------------
Chandler Carruth wrote:
> I'd probably put this in a 'foldPHINodeORSelectInst' static helper so you can write the if below:
>
> if (Value *Result = foldPHINodeOrSelectInst(I)) {
done
================
Comment at: lib/Transforms/Scalar/SROA.cpp:699
@@ -695,5 +698,3 @@
// See if we already have computed info on this node.
- uint64_t &SelectSize = PHIOrSelectSizes[&SI];
- if (!SelectSize) {
- // This is a new Select, check for an unsafe use of it.
- if (Instruction *UnsafeI = hasUnsafePHIOrSelectUse(&SI, SelectSize))
+ uint64_t &PHIOrSelectSize = PHIOrSelectSizes[&I];
+ if (!PHIOrSelectSize) {
----------------
Chandler Carruth wrote:
> How about just "Size"?
done.
================
Comment at: test/Transforms/SROA/phi-and-select.ll:567-568
@@ +566,3 @@
+}
+; CHECK-LABEL: @simplify_phi_nodes_that_equal_slice_2(
+; CHECK-NOT: alloca
----------------
Chandler Carruth wrote:
> I like to have all my CHECKs within the function body so I can find them and they don't get spliced. I also try to attach them to the instruction they reference. Can you put the label at the start of the function and then the -NOT after the alloca?
done
http://reviews.llvm.org/D4659
More information about the llvm-commits
mailing list