[GVN] Prevent load coercion with irregular vector types

[davide]

Summary

See https://llvm.org/bugs/show_bug.cgi?id=28879 for reference

[majnemer]

lib/Transforms/Scalar/GVN.cpp

724

Isn't this llvm::alignTo? Any thoughts on sorting this check before the previous one? I see this check as a sort of sanity check.

[davide]

Yes, it's alignTo (didn't look at MathExtras.h before this commit, sorry =)). Reordered the check. Does it look OK to you?
BTW, RKSimon gave LGTM offline (forgot to mention).

[reames]

lib/Transforms/Scalar/GVN.cpp

721

StoreSize % 8 == 0 would seem to be a more standard way to express this check.

Also, checking only the StoreSize seems suspect. Why do we not need to test the load size?

783

Have you looked at what's required to fix this code path? This should be the only logic protected by the predicate you're modifying.

test/Transforms/GVN/i7vector.ll

7

Using a bitcast might be more canonical here.

[davide]

lib/Transforms/Scalar/GVN.cpp

721

I think checking also the LoadSize is sensible.

783

No, I didn't. Personally I looked at this because I got an internal report of a crash and I bothered to fix it.
The test is generated by a fuzzer so I'm not entirely sure how likely is to hit a situation like this in the wild, but still I thought it was worth fixing. I'm working with Daniel on NewGVN and I'd rather focus my efforts on that rather than fixing the load coercion system of some code I hope will go away in the foreseeable future.

[davide]

lib/Transforms/Scalar/GVN.cpp

783

Hi Philip, in the light of what I said (i.e. that I'm not entirely sure it's worth working on fixing this problem) are you OK with me updating the patch (i.e. adding the check) and going forward?
If so, I can spend some more time on this patch. Otherwise I'll abandon to clean up my queue.

[• dberlin]

I think this patch is fine for now?
We know that load coercion in GVN has some issues here and there, and i think trying to patch them without reworking them entirely is reasonable ATM.

[reames]

I strongly oppose landing a patch that would not pass our usual review standard just because it is in "old" code. I have made suggestion on how to improve the patch which have not been addressed. Given that, no I am not okay with this patch landing in it's current form.

To be clear, part of the reason I'm reacting so strongly here is that this is not the first patch this has come up on. I'm worried about the broader trend as much as I am the particular instance. Put differently, I'd consider a revised version of this patch with the minor comments addressed and LoadSize handled. I would prefer to see some discussion of why the alternate approach I asked about isn't worthwhile, but I would not hold the patch just for that.

[davide]

In D25849#659445, @reames wrote:

I strongly oppose landing a patch that would not pass our usual review standard just because it is in "old" code. I have made suggestion on how to improve the patch which have not been addressed. Given that, no I am not okay with this patch landing in it's current form.

To be clear, part of the reason I'm reacting so strongly here is that this is not the first patch this has come up on. I'm worried about the broader trend as much as I am the particular instance. Put differently, I'd consider a revised version of this patch with the minor comments addressed and LoadSize handled. I would prefer to see some discussion of why the alternate approach I asked about isn't worthwhile, but I would not hold the patch just for that.

FWIW, I can live with the crash. Can you please elaborate what do you mean when you talk about this "recent trend"? Just curious.

[• dberlin]

I guess i disagree that it would pass our usual review standard.
Our usual review standard would have been to disable or revert the thing that added the ability to do this in the first place.
if that fails, we'd turn it off.
Failing that, we'd generally make targeted fixes like this one.

Your argument here seems to be "it doesn't fix enough". Is there a testcase demonstrated for that?

also, if you see a larger trend, i totally thing that is worth bringing up on llvm-dev.
However, realize there's plenty of code there simply are no maintainers for.

[reames]

Ok, I've had a chance to review the code more closely. The alternate approach I'd suggested does look to be a lot more work. More generally, it looks like *all* of the GVN code gives up on non-byte multiple values, this is simply fixing one particular path through that was missing a check.

For the actual change at hand, I am okay with this going in, provided the following is done:

1. Add the check for load size
2. Further reduce the test case
3. Add a test case for a byte multiple store and a non-multiple load
4. Add a test case for load/load forwarding (same problem exists for the def case there as well)

Now, let me turn to the meta discussion.

Danny, I really don't think this patch *does* meet our standards for the following reasons:

1. Comments on the review were ignored. (Checking load size)
2. When asked about alternate approaches (which is pretty much the purpose of review), author did not do so. To be clear, I'm perfectly fine if the alternate approach is *rejected after consideration*, but that *after consideration* part is important.
3. There is no discussion in the review of *what issue is being fixed*. Something simple along the lines of "GVN doesn't handle non byte multiple sizes. We correctly protect the clobber paths, but missed a case on the def path." would have gone a long way to simplifying the review here. Also, there was no discussion in the *review* that this was a regression.
4. Patch does not consider alternate ways of triggering essentially the same bug. (i.e. the load case) Test cases were extremely minimal and didn't provide good coverage of the previous broken code. It is not a strict *requirement* that adjacent bugs be fixed, but it is certainly *good practice* to look for them when the first problem report comes in.

The "trend" I was referencing was specifically in reference to a previous GVN patch (hoisting non-locals to entry block) which the author chose to revert rather than discuss when asked about it.

"I'm busy with other things" is not a reason to relax our usual review standards. There are days I wish it was - it would certainly make my life easier - but from the perspective of the project accepting bad code because the author is busy is a deeply dangerous idea.

[davide]

In D25849#661773, @reames wrote:

Ok, I've had a chance to review the code more closely. The alternate approach I'd suggested does look to be a lot more work. More generally, it looks like *all* of the GVN code gives up on non-byte multiple values, this is simply fixing one particular path through that was missing a check.

For the actual change at hand, I am okay with this going in, provided the following is done:

1. Add the check for load size

Do we really need this?

2. Further reduce the test case

Ditto (it's as 7 lines testcase).

3. Add a test case for a byte multiple store and a non-multiple load

I don't see this asked in the original review.

4. Add a test case for load/load forwarding (same problem exists for the def case there as well)

Ditto.

Now, let me turn to the meta discussion.

Danny, I really don't think this patch *does* meet our standards for the following reasons:

1. Comments on the review were ignored. (Checking load size)

Not quite. See my previous reply:
"are you OK with me updating the patch (i.e. adding the check) and going forward?"

2. When asked about alternate approaches (which is pretty much the purpose of review), author did not do so. To be clear, I'm perfectly fine if the alternate approach is *rejected after consideration*, but that *after consideration* part is important.

And I pointed out it's not worth my time (but you can disagree, and fix that yourself) fixing load coercion for vectors of irregular types. Crashes are bad, IMHO, so I decided to take a look. Easy peasy.
Please note that the current patch is already an improvement on the current state of things, as in, hey, at least we don't crash anymore and we don't provide a wrong result.

3. There is no discussion in the review of *what issue is being fixed*. Something simple along the lines of "GVN doesn't handle non byte multiple sizes. We correctly protect the clobber paths, but missed a case on the def path." would have gone a long way to simplifying the review here. Also, there was no discussion in the *review* that this was a regression.

ehm, this is fixing a crash.

4. Patch does not consider alternate ways of triggering essentially the same bug. (i.e. the load case) Test cases were extremely minimal and didn't provide good coverage of the previous broken code. It is not a strict *requirement* that adjacent bugs be fixed, but it is certainly *good practice* to look for them when the first problem report comes in.

This patch is fixing a bug. Related bugs can be considered (if I'd really choose to do so) as follow-ups. We do that in LLVM all the time.

The "trend" I was referencing was specifically in reference to a previous GVN patch (hoisting non-locals to entry block) which the author chose to revert rather than discuss when asked about it.

"I'm busy with other things" is not a reason to relax our usual review standards. There are days I wish it was - it would certainly make my life easier - but from the perspective of the project accepting bad code because the author is busy is a deeply dangerous idea.

Not my words =) If I was you I would be more careful with quotes =) Also, what is the bad code?

[davide]

In D25849#661788, @davide wrote:

In D25849#661773, @reames wrote:

Ok, I've had a chance to review the code more closely. The alternate approach I'd suggested does look to be a lot more work. More generally, it looks like *all* of the GVN code gives up on non-byte multiple values, this is simply fixing one particular path through that was missing a check.

For the actual change at hand, I am okay with this going in, provided the following is done:

1. Add the check for load size

Do we really need this?

2. Further reduce the test case

Ditto (it's as 7 lines testcase).

3. Add a test case for a byte multiple store and a non-multiple load

I don't see this asked in the original review.

4. Add a test case for load/load forwarding (same problem exists for the def case there as well)

Ditto.

Now, let me turn to the meta discussion.

Danny, I really don't think this patch *does* meet our standards for the following reasons:

1. Comments on the review were ignored. (Checking load size)

Not quite. See my previous reply:
"are you OK with me updating the patch (i.e. adding the check) and going forward?"

2. When asked about alternate approaches (which is pretty much the purpose of review), author did not do so. To be clear, I'm perfectly fine if the alternate approach is *rejected after consideration*, but that *after consideration* part is important.

And I pointed out it's not worth my time (but you can disagree, and fix that yourself) fixing load coercion for vectors of irregular types. Crashes are bad, IMHO, so I decided to take a look. Easy peasy.
Please note that the current patch is already an improvement on the current state of things, as in, hey, at least we don't crash anymore and we don't provide a wrong result.

3. There is no discussion in the review of *what issue is being fixed*. Something simple along the lines of "GVN doesn't handle non byte multiple sizes. We correctly protect the clobber paths, but missed a case on the def path." would have gone a long way to simplifying the review here. Also, there was no discussion in the *review* that this was a regression.

ehm, this is fixing a crash.

4. Patch does not consider alternate ways of triggering essentially the same bug. (i.e. the load case) Test cases were extremely minimal and didn't provide good coverage of the previous broken code. It is not a strict *requirement* that adjacent bugs be fixed, but it is certainly *good practice* to look for them when the first problem report comes in.

This patch is fixing a bug. Related bugs can be considered (if I'd really choose to do so) as follow-ups. We do that in LLVM all the time.

The "trend" I was referencing was specifically in reference to a previous GVN patch (hoisting non-locals to entry block) which the author chose to revert rather than discuss when asked about it.

Note that I also never ignored any comments, while I'm still waiting for a reply from you from Oct28th =)

[davide]

In D25849#661821, @davide wrote:

In D25849#661788, @davide wrote:

In D25849#661773, @reames wrote:

Ok, I've had a chance to review the code more closely. The alternate approach I'd suggested does look to be a lot more work. More generally, it looks like *all* of the GVN code gives up on non-byte multiple values, this is simply fixing one particular path through that was missing a check.

For the actual change at hand, I am okay with this going in, provided the following is done:

1. Add the check for load size

Do we really need this?

2. Further reduce the test case

Ditto (it's as 7 lines testcase).

3. Add a test case for a byte multiple store and a non-multiple load

I don't see this asked in the original review.

4. Add a test case for load/load forwarding (same problem exists for the def case there as well)

Ditto.

Now, let me turn to the meta discussion.

Danny, I really don't think this patch *does* meet our standards for the following reasons:

1. Comments on the review were ignored. (Checking load size)

Not quite. See my previous reply:
"are you OK with me updating the patch (i.e. adding the check) and going forward?"

2. When asked about alternate approaches (which is pretty much the purpose of review), author did not do so. To be clear, I'm perfectly fine if the alternate approach is *rejected after consideration*, but that *after consideration* part is important.

And I pointed out it's not worth my time (but you can disagree, and fix that yourself) fixing load coercion for vectors of irregular types. Crashes are bad, IMHO, so I decided to take a look. Easy peasy.
Please note that the current patch is already an improvement on the current state of things, as in, hey, at least we don't crash anymore and we don't provide a wrong result.

3. There is no discussion in the review of *what issue is being fixed*. Something simple along the lines of "GVN doesn't handle non byte multiple sizes. We correctly protect the clobber paths, but missed a case on the def path." would have gone a long way to simplifying the review here. Also, there was no discussion in the *review* that this was a regression.

ehm, this is fixing a crash.

4. Patch does not consider alternate ways of triggering essentially the same bug. (i.e. the load case) Test cases were extremely minimal and didn't provide good coverage of the previous broken code. It is not a strict *requirement* that adjacent bugs be fixed, but it is certainly *good practice* to look for them when the first problem report comes in.

This patch is fixing a bug. Related bugs can be considered (if I'd really choose to do so) as follow-ups. We do that in LLVM all the time.

The "trend" I was referencing was specifically in reference to a previous GVN patch (hoisting non-locals to entry block) which the author chose to revert rather than discuss when asked about it.

Note that I also never ignored any comments, while I'm still waiting for a reply from you from Oct28th =)

And last I checked, reverting dubious patches/patches where objections are raised is not an uncommon practice in LLVM (in fact, IIRC, we encourage early revert in case there are problems, e.g. bots broken, issues in the design). I also would like to point out it's an isolated case. And I actually proposed to work an alternative, that is NewGVN (which actually was committed shortly after, and it's going very strong, thanks to the burst of recent commits from Danny).

[reames]

In D25849#661788, @davide wrote:

In D25849#661773, @reames wrote:

Ok, I've had a chance to review the code more closely. The alternate approach I'd suggested does look to be a lot more work. More generally, it looks like *all* of the GVN code gives up on non-byte multiple values, this is simply fixing one particular path through that was missing a check.

For the actual change at hand, I am okay with this going in, provided the following is done:

1. Add the check for load size

Do we really need this?

2. Further reduce the test case

Ditto (it's as 7 lines testcase).

3. Add a test case for a byte multiple store and a non-multiple load

I don't see this asked in the original review.

4. Add a test case for load/load forwarding (same problem exists for the def case there as well)

Ditto.

I'm willing to consider a patch with a subset of these, but I'm not going to discuss this abstractly. If you want me to consider a patch, please update the review.

Now, let me turn to the meta discussion.

Danny, I really don't think this patch *does* meet our standards for the following reasons:

1. Comments on the review were ignored. (Checking load size)

Not quite. See my previous reply:
"are you OK with me updating the patch (i.e. adding the check) and going forward?"

You are correct about the comment, but the patch has never been updated. Given that, the discussion about whether it can be committed is premature.

2. When asked about alternate approaches (which is pretty much the purpose of review), author did not do so. To be clear, I'm perfectly fine if the alternate approach is *rejected after consideration*, but that *after consideration* part is important.

And I pointed out it's not worth my time (but you can disagree, and fix that yourself) fixing load coercion for vectors of irregular types. Crashes are bad, IMHO, so I decided to take a look. Easy peasy.

Asking for an alternate approach to be considered is normal part of review.

Please note that the current patch is already an improvement on the current state of things, as in, hey, at least we don't crash anymore and we don't provide a wrong result.

We have a long history of not accepting low quality patches, even for crashes.

3. There is no discussion in the review of *what issue is being fixed*. Something simple along the lines of "GVN doesn't handle non byte multiple sizes. We correctly protect the clobber paths, but missed a case on the def path." would have gone a long way to simplifying the review here. Also, there was no discussion in the *review* that this was a regression.

ehm, this is fixing a crash.

That is not mentioned in the review description. It is only in the linked bug. Explaining *how/why* the crash happened is expected and required.

4. Patch does not consider alternate ways of triggering essentially the same bug. (i.e. the load case) Test cases were extremely minimal and didn't provide good coverage of the previous broken code. It is not a strict *requirement* that adjacent bugs be fixed, but it is certainly *good practice* to look for them when the first problem report comes in.

This patch is fixing a bug. Related bugs can be considered (if I'd really choose to do so) as follow-ups. We do that in LLVM all the time.

The "trend" I was referencing was specifically in reference to a previous GVN patch (hoisting non-locals to entry block) which the author chose to revert rather than discuss when asked about it.

"I'm busy with other things" is not a reason to relax our usual review standards. There are days I wish it was - it would certainly make my life easier - but from the perspective of the project accepting bad code because the author is busy is a deeply dangerous idea.

Not my words =) If I was you I would be more careful with quotes =) Also, what is the bad code?

My apologies. I did quote where I shouldn't have. That was the way I read your response, but not words you actually said.

Merging other comments so I can reply in one...

Note that I also never ignored any comments, while I'm still waiting for a reply from you from Oct28th =)

You're correct that in an ideal world I should have replied sooner. I will point out that I volunteer my time for things like this and do not promise to ever respond to a given review thread unless it looks like a good use of my time.

And last I checked, reverting dubious patches/patches where objections are raised is not an uncommon practice in LLVM (in fact, IIRC, we encourage early revert in case there are problems, e.g. bots broken, issues in the design). I also would like to point out it's an isolated case.

Ok, you've brought up this point a couple of times. I assume you're referring to the refactoring change from early last year which exposed this crash? If you'd proposed a revert of patch which a clear test case, I most likely would not have objected. You did not do this. You posted a patch without clear context and then failed to respond to review comments with a revised patch.

And I actually proposed to work an alternative, that is NewGVN (which actually was committed shortly after, and it's going very strong, thanks to the burst of recent commits from Danny).

As I said before, working on an alternative solution is not a reason to accept low quality patches for the infrastructure currently in use.

[davide]

In D25849#664119, @reames wrote:

In D25849#661788, @davide wrote:

In D25849#661773, @reames wrote:

Ok, I've had a chance to review the code more closely. The alternate approach I'd suggested does look to be a lot more work. More generally, it looks like *all* of the GVN code gives up on non-byte multiple values, this is simply fixing one particular path through that was missing a check.

For the actual change at hand, I am okay with this going in, provided the following is done:

1. Add the check for load size

Do we really need this?

2. Further reduce the test case

Ditto (it's as 7 lines testcase).

3. Add a test case for a byte multiple store and a non-multiple load

I don't see this asked in the original review.

4. Add a test case for load/load forwarding (same problem exists for the def case there as well)

Ditto.

I'm willing to consider a patch with a subset of these, but I'm not going to discuss this abstractly. If you want me to consider a patch, please update the review.

Now, let me turn to the meta discussion.

Danny, I really don't think this patch *does* meet our standards for the following reasons:

1. Comments on the review were ignored. (Checking load size)

Not quite. See my previous reply:
"are you OK with me updating the patch (i.e. adding the check) and going forward?"

You are correct about the comment, but the patch has never been updated. Given that, the discussion about whether it can be committed is premature.

2. When asked about alternate approaches (which is pretty much the purpose of review), author did not do so. To be clear, I'm perfectly fine if the alternate approach is *rejected after consideration*, but that *after consideration* part is important.

And I pointed out it's not worth my time (but you can disagree, and fix that yourself) fixing load coercion for vectors of irregular types. Crashes are bad, IMHO, so I decided to take a look. Easy peasy.

Asking for an alternate approach to be considered is normal part of review.

Please note that the current patch is already an improvement on the current state of things, as in, hey, at least we don't crash anymore and we don't provide a wrong result.

We have a long history of not accepting low quality patches, even for crashes.

Let's agree to disagree. You still haven't pointed out anything wrong with the patch itself. Danny already provided an explanation why this is an OK (even if not perfect) fix to accept. That said, low quality code IMHO includes (but not limited to): untested patches (which is not the case here), patch with obvious mistake (again, the check seems correct, here), etc..
With that in mind, I tend to agree that the patch can be extended to catch other cases. I also think that these can be considered separately. As I pretty much have zero interest in discussing this further with you (and I consider it a very poor use of my time), I'll just ignore and go on with my life/work ;)

3. There is no discussion in the review of *what issue is being fixed*. Something simple along the lines of "GVN doesn't handle non byte multiple sizes. We correctly protect the clobber paths, but missed a case on the def path." would have gone a long way to simplifying the review here. Also, there was no discussion in the *review* that this was a regression.

ehm, this is fixing a crash.

That is not mentioned in the review description. It is only in the linked bug. Explaining *how/why* the crash happened is expected and required.

The review description has a link containing the crash. Saying that I didn't copy/pasted that in the review is a little weak as argument.

4. Patch does not consider alternate ways of triggering essentially the same bug. (i.e. the load case) Test cases were extremely minimal and didn't provide good coverage of the previous broken code. It is not a strict *requirement* that adjacent bugs be fixed, but it is certainly *good practice* to look for them when the first problem report comes in.

This patch is fixing a bug. Related bugs can be considered (if I'd really choose to do so) as follow-ups. We do that in LLVM all the time.

The "trend" I was referencing was specifically in reference to a previous GVN patch (hoisting non-locals to entry block) which the author chose to revert rather than discuss when asked about it.

"I'm busy with other things" is not a reason to relax our usual review standards. There are days I wish it was - it would certainly make my life easier - but from the perspective of the project accepting bad code because the author is busy is a deeply dangerous idea.

Not my words =) If I was you I would be more careful with quotes =) Also, what is the bad code?

My apologies. I did quote where I shouldn't have. That was the way I read your response, but not words you actually said.

Merging other comments so I can reply in one...

Note that I also never ignored any comments, while I'm still waiting for a reply from you from Oct28th =)

You're correct that in an ideal world I should have replied sooner. I will point out that I volunteer my time for things like this and do not promise to ever respond to a given review thread unless it looks like a good use of my time.

And last I checked, reverting dubious patches/patches where objections are raised is not an uncommon practice in LLVM (in fact, IIRC, we encourage early revert in case there are problems, e.g. bots broken, issues in the design). I also would like to point out it's an isolated case.

Ok, you've brought up this point a couple of times. I assume you're referring to the refactoring change from early last year which exposed this crash? If you'd proposed a revert of patch which a clear test case, I most likely would not have objected. You did not do this. You posted a patch without clear context and then failed to respond to review comments with a revised patch.

And I actually proposed to work an alternative, that is NewGVN (which actually was committed shortly after, and it's going very strong, thanks to the burst of recent commits from Danny).

As I said before, working on an alternative solution is not a reason to accept low quality patches for the infrastructure currently in use.

lib/Transforms/Scalar/GVN.cpp

                                             const DataLayout &DL) {
   // If the loaded or stored value is an first class array or struct, don't try
   // to transform them.  We need to be able to bitcast to integer.
   if (LoadTy->isStructTy() || LoadTy->isArrayTy() ||
       StoredVal->getType()->isStructTy() ||
       StoredVal->getType()->isArrayTy())
     return false;
 
+  uint64_t StoreSize = DL.getTypeSizeInBits(StoredVal->getType());
+
+  // Prevent load coercion with irregular vector types (e.g. <7 x i1>).
+  // Other parts of the code don't deal with them very well and we're
+  // going to crash later anyway.
+  if (llvm::alignTo(StoreSize, 8) != StoreSize)
+    return false;
+
   // The store has to be at least as big as the load.
-  if (DL.getTypeSizeInBits(StoredVal->getType()) <
-        DL.getTypeSizeInBits(LoadTy))
+  if (StoreSize < DL.getTypeSizeInBits(LoadTy))
     return false;
 
   return true;
 }
 
 /// If we saw a store of a value to memory, and
 /// then a load from a must-aliased pointer of a different type, try to coerce
 /// the stored value.  LoadedTy is the type of the load we want to replace.

     return StoredVal;
   }
 
   // If the loaded value is smaller than the available value, then we can
   // extract out a piece from it.  If the available value is too small, then we
   // can't do anything.
   assert(StoredValSize >= LoadedValSize &&
          "CanCoerceMustAliasedValueToLoad fail");
 
   // Convert source pointers to integers, which can be manipulated.
   if (StoredValTy->getScalarType()->isPointerTy()) {
     StoredValTy = DL.getIntPtrType(StoredValTy);
     StoredVal = IRB.CreatePtrToInt(StoredVal, StoredValTy);
   }
 
   // Convert vectors and fp to integer, which can be manipulated.
   if (!StoredValTy->isIntegerTy()) {

test/Transforms/GVN/i7vector.ll

+; RUN: opt -gvn %s -S -o -
+
+define void @f() {
+entry:
+  %a = alloca <7 x i1>, align 2
+  store <7 x i1> undef, <7 x i1>* %a, align 2
+  %0 = getelementptr inbounds <7 x i1>, <7 x i1>* %a, i64 0, i64 0
+  %val = load i1, i1* %0, align 2
+  br i1 %val, label %cond.true, label %cond.false
+
+cond.true:
+  ret void
+
+cond.false:
+  ret void
+}