[PATCH] D20262: [DSE]Split memset when the memset is small enough to be lowered to stores

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Tue May 31 16:30:49 PDT 2016


+Mehdi, who set the mandatory data layout.

> On May 27, 2016, at 7:52 AM, Jun Bum Lim <junbuml at codeaurora.org> wrote:
> 
> junbuml added a comment.
> 
> Thanks Quentin for the review. I will address your comments soon.
> 
> 
> ================
> Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:238
> @@ +237,3 @@
> +
> +  if (MaxIntSize == 0)
> +    MaxIntSize = 1;
> ----------------
> qcolombet wrote:
>> junbuml wrote:
>>> dberlin wrote:
>>>> How and when can this happen?
>>> If there is no information about LegalIntWidth extracted from datalayout, this could be 0.  If we remove the target datalayout in input IR, this will be 0.
>> Like Daniel said, DL are mandatory now. If that may still happen, (int would be an illegal type??) add a comment on when this is the case and add a test case for that!
> As far as I check, I didn't encounter any complain when injecting an IR without "target datalayout" into opt. Also there are many test cases without datalayout. It seems also possible to assign empty string like target datalayout="". Did the mandatory mean in IRs generated from the frontend? 

Mehdi, could you clarify that point?
Moreover, shouldn’t the IR verifier complain if the data layout string is missing?

> 
> If DL.getLargestLegalIntTypeSizeInBits() returns 0, I think we should bail out with the assumption that we don't have proper information about backend.
> 
> 
> http://reviews.llvm.org/D20262
> 
> 
> 



More information about the llvm-commits mailing list