[PATCH] D42230: [CGP] Fix the GV handling in complex addressing mode

[John Brawn via Phabricator via llvm-commits]

john.brawn added a comment.

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.



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2708-2713
     if (DifferentField != ExtAddrMode::MultipleFields &&
         DifferentField != ExtAddrMode::ScaleField &&
         (DifferentField != ExtAddrMode::BaseOffsField ||
-         !NewAddrMode.ScaledReg)) {
+         !NewAddrMode.ScaledReg) &&
+        (DifferentField != ExtAddrMode::BaseGVField ||
+         !NewAddrMode.HasBaseReg)) {
----------------
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;
```


================
Comment at: test/Transforms/CodeGenPrepare/X86/sink-addrmode-select.ll:2-5
+;target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128"
+;target triple = "x86_64-unknown-linux-gnu"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
----------------
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).


https://reviews.llvm.org/D42230