[CGP] Fix the GV handling in complex addressing mode

[skatkov]

Summary

If in complex addressing mode the difference is in GV then
base reg should not be installed because we plan to use
base reg as a merge point of different GVs.

This is a fix for PR35980.

[john.brawn]

It looks like the reason we get both BaseGV and BaseReg is that matchScaledValue will use BaseReg when the scale is 1 (and BaseReg isn't already used). It may be worthwhile making it consistently use ScaledReg, as then I don't think we would run into this problem, but fixing things so we don't fail an assertion if we do happen to have both is definitely something we should do.

lib/CodeGen/CodeGenPrepare.cpp

2708–2713 ↗

(On Diff #130362)

I think that the logic has gotten complex enough here that it would be clearer to split it up and invert it so we sequentially check for all the things that can't be handled (each with its own comment), something like:

bool CanAdd = true;
// If NewAddrMode differs in only one dimension then we can handle it by...
if (DifferentField == ExtAddrMode::MultipleFields)
  CanAdd = false;
// We can't handle ScaleField
if (DifferentField == ExtAddrMode::ScaleField)
  CanAdd = false;
// We also must reject ...
if (DifferentField == ExtAddrMode::BaseOffsField && NewAddrMode.ScaledReg)
  CanAdd = false;
// We also must reject ...
if (DifferentField == ExtAddrMode::BaseGVField && NewAddrMode.HasBaseReg)
  CanAdd = false;

if (CanAdd)
  AddrModes.emplace_back(NewAddrMode);
else
  AddrModes.clear();

return CanAdd;

test/Transforms/CodeGenPrepare/X86/sink-addrmode-select.ll

2–5 ↗

(On Diff #130362)

It doesn't look like changing the datalayout has any effect on the test (and you shouldn't be doing it by commenting out the old one anyway).

[skatkov]

Please take a look.

[john.brawn]

LGTM.

llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp

bool addNewAddrMode(ExtAddrMode &NewAddrMode) {
     // about the cumulative difference.
     ExtAddrMode::FieldName ThisDifferentField =
       AddrModes[0].compare(NewAddrMode);
     if (DifferentField == ExtAddrMode::NoField)
       DifferentField = ThisDifferentField;
     else if (DifferentField != ThisDifferentField)
       DifferentField = ExtAddrMode::MultipleFields;
 
-    // If NewAddrMode differs in only one dimension, and that dimension isn't
-    // the amount that ScaledReg is scaled by, then we can handle it by
-    // inserting a phi/select later on. Even if NewAddMode is the same
-    // we still need to collect it due to original value is different.
-    // And later we will need all original values as anchors during
-    // finding the common Phi node.
+    // If NewAddrMode differs in more than one dimension we cannot handle it.
+    bool CanHandle = DifferentField != ExtAddrMode::MultipleFields;
+
+    // If Scale Field is different then we reject.
+    CanHandle = CanHandle && DifferentField != ExtAddrMode::ScaleField;
+
     // We also must reject the case when base offset is different and
     // scale reg is not null, we cannot handle this case due to merge of
     // different offsets will be used as ScaleReg.
-    if (DifferentField != ExtAddrMode::MultipleFields &&
-        DifferentField != ExtAddrMode::ScaleField &&
-        (DifferentField != ExtAddrMode::BaseOffsField ||
-         !NewAddrMode.ScaledReg)) {
-      AddrModes.emplace_back(NewAddrMode);
-      return true;
-    }
+    CanHandle = CanHandle && (DifferentField != ExtAddrMode::BaseOffsField ||
+                              !NewAddrMode.ScaledReg);
 
-    // We couldn't combine NewAddrMode with the rest, so return failure.
+    // We also must reject the case when GV is different and BaseReg installed
+    // due to we want to use base reg as a merge of GV values.
+    CanHandle = CanHandle && (DifferentField != ExtAddrMode::BaseGVField ||
+                              !NewAddrMode.HasBaseReg);
+
+    // Even if NewAddMode is the same we still need to collect it due to
+    // original value is different. And later we will need all original values
+    // as anchors during finding the common Phi node.
+    if (CanHandle)
+      AddrModes.emplace_back(NewAddrMode);
+    else
       AddrModes.clear();
-    return false;
+
+    return CanHandle;
   }
 
   /// \brief Combine the addressing modes we've collected into a single
   /// addressing mode.
   /// \return True iff we successfully combined them or we only had one so
   /// didn't need to combine them anyway.
   bool combineAddrModes() {
     // If we have no AddrModes then they can't be combined.

llvm/trunk/test/Transforms/CodeGenPrepare/X86/sink-addrmode-select.ll

; CHECK-LABEL: entry:
   %g1 = getelementptr inbounds i64, i64* %g, i64 8
   %g2 = getelementptr inbounds i64, i64* %g, i64 16
   %s = select i1 %c, i64* %g1, i64* %g2
 ; CHECK-NOT: sunkaddr
   %v = load i64 , i64* %s, align 8
   ret i64 %v
 }
 
+@gv1 = external global i8, align 16
+@gv2 = external global i8, align 16
+
+; Select when both GV and base reg are present.
+define i8 @test2(i1 %c, i64 %b) {
+; CHECK-LABEL: @test2
+entry:
+; CHECK-LABEL: entry:
+  %g1 = getelementptr inbounds i8, i8* @gv1, i64 %b
+  %g2 = getelementptr inbounds i8, i8* @gv2, i64 %b
+  %s = select i1 %c, i8* %g1, i8* %g2
+; CHECK-NOT: sunkaddr
+  %v = load i8 , i8* %s, align 8
+  ret i8 %v
+}